-
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: Remove unecessary code from async and sync select components #20690
chore: Remove unecessary code from async and sync select components #20690
Conversation
Changed files to reference AsyncSelect if needed
…LDN-1484-Split-Select-Component-into-Async-and-Sync-Components
…LDN-1484-Split-Select-Component-into-Async-and-Sync-Components
…c-and-Sync-Components
…LDN-1484-Split-Select-Component-into-Async-and-Sync-Components
…ponents' and 'master' of github.com:CybercentreCanada/superset into Remove-Unecessary-Code-from-Async-and-Sync-Select-Components
…emove-Unecessary-Code-from-Async-and-Sync-Select-Components
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.
Thank you for the PR @cccs-RyanK! I left some first-pass comments about the sync version. I'll review the async version later 😉
@cccs-RyanK About your question on Slack... I think it's a good idea to clean the components first and extract shared behaviors in another PR. It will be easier to check what can be extracted after the cleanup and it will be easier to review the changes. |
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.
@cccs-RyanK Thank you for the test improvements!
…emove-Unecessary-Code-from-Async-and-Sync-Select-Components
…emove-Unecessary-Code-from-Async-and-Sync-Select-Components
Codecov Report
@@ Coverage Diff @@
## master #20690 +/- ##
==========================================
- Coverage 66.29% 66.26% -0.04%
==========================================
Files 1758 1758
Lines 66801 66930 +129
Branches 7055 7088 +33
==========================================
+ Hits 44288 44349 +61
- Misses 20713 20770 +57
- Partials 1800 1811 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. |
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.
Although showSearch
is true
by default, it can be disabled.
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.
LGTM. Thank you for all the hard work @cccs-RyanK!
SUMMARY
This task was outlined as a part of this PR by @michael-s-molina : #20143
In a previous PR, the code for Select and AsyncSelect was split into two separate components: #20466
The components were left identical, so this PR continues the work and removes the unecessary code that was left in each of the components. Namely, it removes behaviour that had previously been determined by whether the component was in async mode or not.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION