refactor: migrate ExploreResultsButton component to FC & tsx

[EugeneTorap]

SUMMARY

Migrated ExploreResultsButton to TypeScript to apply direction outlined in #18100 .

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue: Migrate components to TypeScript #18100
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[EugeneTorap]

@ad-m @AAfghahi renderInvalidColumnMessage, renderTimeoutWarning, buildVizOptions are not used anywhere.
Should I remove them?

[ad-m]

@ad-m @AAfghahi renderInvalidColumnMessage, renderTimeoutWarning, buildVizOptions are not used anywhere. Should I remove them?

I do not feel entitled to judge the issue of style in this project (I have too little experience in the project), but due tagging, I will express my opinion: dead code should be removed from the codebase, and besides, it is probably removed from bundles provided to the user by the Webpack.

[AAfghahi]

Hello! We can set up a test environment to see if it affects the functionality of the component, if it does not then we should remove them, I agree.

[ad-m]

@EugeneTorap you have some linting errors on CI. You might wanna install pre-commit to avoid committing code like that. Just pre-commit install in the project directory to setup Git hooks. In addition, pre-commit speed-up linting code thanks to linting only changed files.

[EugeneTorap]

@ad-m @AAfghahi Can you review it again and set up a test environment for it?

[AAfghahi]

Oh I think this is already happening here: #17987

[EugeneTorap]

@AAfghahi This #17987 PR only converts code to FC and there's no typescript

[EugeneTorap]

@lyndsiWilliams Can you help me to fix ExploreResultsButton.test.jsx and also convert this test to typescript?

[lyndsiWilliams]

@lyndsiWilliams Can you help me to fix ExploreResultsButton.test.jsx and also convert this test to typescript?

Hey @EugeneTorap! What error are you getting when you run the test?



As far as conversion: My conversion method for JS to TS in these tests is to change the file to .tsx then fix all the errors, making sure to reuse any pre-existing types in the codebase. Are you running into a specific error in your conversion? We’re also trying to convert all of our Enzyme tests to React testing library. If you’re interested, you could take the TS/RTL conversion to a new PR for this test and I’d be delighted to give you some pointers.

[EugeneTorap]

@lyndsiWilliams

Error: useTheme() could not find a ThemeContext. The component is likely missing from the app.

[lyndsiWilliams]

@lyndsiWilliams

Error: useTheme() could not find a ThemeContext. The component is likely missing from the app.

You'll need to wrap each render in a <ThemeProvider />, as seen here in ExploreActionButtons.test.jsx. Upon further inspection, this test will need to be converted to RTL since this is a functional component now. The Enzyme tests are set up to test class function-only behavior like props, so these will now be broken. We've got some handy helper functions set up for our RTL tests in the spec/helpers file that will provide the theme (and other handy things if needed) for you.

[codecov]

Codecov Report

Merging #18143 (785c2fe) into master (0cec0c9) will increase coverage by 0.27%.
The diff coverage is n/a.

❗ Current head 785c2fe differs from pull request most recent head e065cee. Consider uploading reports for the commit e065cee to get more accurate results

@@            Coverage Diff             @@
##           master   #18143      +/-   ##
==========================================
+ Coverage   65.95%   66.23%   +0.27%     
==========================================
  Files        1584     1594      +10     
  Lines       62046    62586     +540     
  Branches     6273     6304      +31     
==========================================
+ Hits        40920    41451     +531     
+ Misses      19505    19490      -15     
- Partials     1621     1645      +24     

Flag	Coverage Δ
javascript	51.31% <ø> (+0.43%)	⬆️
presto	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engine_specs/teradata.py	62.75% <0.00%> (-27.25%)	⬇️
...rontend/src/explore/components/EmbedCodeButton.jsx	66.66% <0.00%> (-12.29%)	⬇️
superset/key_value/api.py	81.52% <0.00%> (-9.97%)	⬇️
...dashboard/components/menu/ShareMenuItems/index.tsx	65.38% <0.00%> (-8.53%)	⬇️
...nd/src/explore/components/ExploreActionButtons.tsx	57.14% <0.00%> (-5.72%)	⬇️
superset/db_engine_specs/presto.py	84.10% <0.00%> (-5.03%)	⬇️
...rc/explore/components/controls/TextAreaControl.jsx	80.00% <0.00%> (-4.22%)	⬇️
.../src/dashboard/components/gridComponents/Chart.jsx	58.88% <0.00%> (-4.21%)	⬇️
superset/views/dashboard/views.py	65.16% <0.00%> (-4.07%)	⬇️
superset/security/api.py	96.49% <0.00%> (-3.51%)	⬇️
... and 126 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cec0c9...e065cee. Read the comment docs.

[EugeneTorap]

@geido @AAfghahi @lyndsiWilliams Can you review the PR again.
And I think we can close #17987

[geido]

/testenv up

[github-actions]

@geido Ephemeral environment spinning up at http://34.220.37.167:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[geido]

I am tentatively approving this PR but it would be great to move the tests to RTL if possible. I am fine with doing that in a separate PR as long as we don't lose track of those. Thank you

[AAfghahi]

I agree with Geido, that it would be ideal if these tests were written in RTL. But happy to merge this.

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.