-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: refactor ResultSet to functional component #21186
Conversation
Storybook has completed and can be viewed at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the conversion! I found a few things that need to be adjusted - also now that this component is functional, its test file will need to be converted to React Testing Library so that the frontend tests can pass.
Storybook has completed and can be viewed at |
Storybook has completed and can be viewed at |
Storybook has completed and can be viewed at |
Codecov Report
@@ Coverage Diff @@
## master #21186 +/- ##
==========================================
+ Coverage 66.41% 66.42% +0.01%
==========================================
Files 1784 1784
Lines 68185 68196 +11
Branches 7265 7270 +5
==========================================
+ Hits 45284 45301 +17
+ Misses 21032 21027 -5
+ Partials 1869 1868 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Storybook has completed and can be viewed at |
2 similar comments
Storybook has completed and can be viewed at |
Storybook has completed and can be viewed at |
superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for the conversion, this is great 😁
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://54.188.192.217:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have some additional testing from QA before merging
Sure, love to test it! |
/testenv up |
@kgabryje Ephemeral environment spinning up at http://35.86.88.190:8080. Credentials are |
/testenv up |
@kgabryje Ephemeral environment spinning up at http://35.91.158.0:8080. Credentials are |
The changes look good to me. @jinghua-qa Would you like to test it? The test env http://35.91.158.0:8080/ contains a fix for diappearing results when switching tabs |
QA tested and verified. LGTM |
@kgabryje @lyndsiWilliams @hughhhh Can we merge it? |
@hughhhh I've overriden your change request since it was about waiting for QA, and that's done. Thanks @andrey-zayats ! |
Ephemeral environment shutdown and build artifacts deleted. |
…)" This reverts commit f603295.
SUMMARY
Converted ResultSet class component into functional component.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION