-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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: Select component refactoring - FilterControl - Iteration 5 #15777
chore: Select component refactoring - FilterControl - Iteration 5 #15777
Conversation
…e/select-component-filtercontrol-iteration-5
Codecov Report
@@ Coverage Diff @@
## master #15777 +/- ##
==========================================
+ Coverage 76.90% 76.92% +0.02%
==========================================
Files 1007 1007
Lines 54109 54087 -22
Branches 7369 7360 -9
==========================================
- Hits 41612 41608 -4
+ Misses 12257 12239 -18
Partials 240 240
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
.../src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/index.jsx
Outdated
Show resolved
Hide resolved
…l/AdhocFilterEditPopoverSqlTabContent/index.jsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
…e/select-component-filtercontrol-iteration-5
/testenv up |
@geido Ephemeral environment spinning up at http://34.209.229.173:8080. Credentials are |
…e/select-component-filtercontrol-iteration-5
…e/select-component-filtercontrol-iteration-5
/testenv up |
@michael-s-molina Ephemeral environment spinning up at http://54.184.73.197:8080. Credentials are |
I think we need to clear the filter value when changing the column. Screen.Recording.2021-09-15.at.6.19.33.PM.mov |
The code lgtm. But in regards to clearing the fields like @michael-s-molina suggested would be nice but to be fair it doesn't currently do that on master so I think that could be done as a follow up as that might require a little more refactoring work? |
@michael-s-molina Just tested on master. As @pkdotson reported this is currently not implemented. I think it makes sense to tackle that in a separate PR as it would be slightly unrelated to the objective of this PR. I'll work on that as soon as we merge this PR. |
Ok. Approving with the follow-up in mind. |
Ephemeral environment shutdown and build artifacts deleted. |
…ache#15777) * Refactor Select for AdhocFilterEditPopoverSqlTabContent * Refactor Selects * Fix numeric options * Clean up * Fix Select value * Add showSearch * Display null label * Implement StyledSelect * Update superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/index.jsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Fix aria-label * Revert MetricControls changes * Update ariaLabel * Reconcile with latest Select changes Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
…ache#15777) * Refactor Select for AdhocFilterEditPopoverSqlTabContent * Refactor Selects * Fix numeric options * Clean up * Fix Select value * Add showSearch * Display null label * Implement StyledSelect * Update superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSqlTabContent/index.jsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Fix aria-label * Revert MetricControls changes * Update ariaLabel * Reconcile with latest Select changes Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
SUMMARY
It replaces the react-select Select component with the Antdesign one in the FilterControl components.
BEFORE/AFTER
AdhocFilterEditPopoverSimpleTabContent.-.BEFORE.mp4
AdhocFilterEditPopoverSimpleTabContent.-.AFTER-new.mp4
BEFORE/AFTER
AdhocFilterEditPopoverSqlTabContent.-.BEFORE.mp4
AdhocFilterEditPopoverSimpleTabContent.-.AFTER.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION