-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
fix(explore): DndColumnSelect not handling controls with "multi: false" #14737
Conversation
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 - one small improvement proposal
@@ -77,6 +78,7 @@ export const DndColumnSelect = (props: LabelProps) => { | |||
canDrop={canDrop} | |||
valuesRenderer={valuesRenderer} | |||
accept={DndItemType.Column} | |||
displayGhostButton={multi || optionSelector.values.length === 0} |
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.
Bycatch: could we add singular/plural support while we're at it? ghostButtonText={tn('Drop column', 'Drop columns', multi ? 2 : 1)}
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.
Codecov Report
@@ Coverage Diff @@
## master #14737 +/- ##
==========================================
- Coverage 77.54% 77.50% -0.04%
==========================================
Files 959 960 +1
Lines 48696 48790 +94
Branches 5743 6126 +383
==========================================
+ Hits 37762 37817 +55
- Misses 10733 10770 +37
- Partials 201 203 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
1d64ec8
to
2ce2e71
Compare
@michael-s-molina @villebro thanks for reviewing! did you test locally? for some reasons i still see the select fields when i pull the PR |
@junlincc Try on Country Map chart. I recorded those videos while implementing dnd controls for all charts (not available on master yet, PR coming in ~30 minutes 🙂) |
* master: (163 commits) fix(native-filters): Manage default value of filters by superset (apache#14785) fix: Additional ResultSet tests (apache#14741) chore: added BasicParametersMixin to Redshift (apache#14752) fix: make dataset list sort case insensitive (apache#14528) fix: use encodeURIComponent when getting table metadata (apache#14790) fix: ensure engine is outside parameters (apache#14787) database modal should close on connect with tab layout (apache#14771) feat(native-filters): add search all filter options (apache#14710) fix: extra query in Dashboard when native filter enabled (apache#14770) chore: Improves the native filters UI/UX - iteration 2 (apache#14753) fix(native filters): Fix explore state (apache#14779) fix(explore): DndColumnSelect not handling controls with "multi: false" (apache#14737) feat: Create BigQuery Parameters for DatabaseModal (apache#14721) feat: enable user impersonation in GSheets (apache#14767) fix: add DB should not say it's Postgres (apache#14766) Revert "fix(dashboard): multiple query trigger when native filter enabled (apache#14734)" (apache#14762) feat: save database with new dynamic form (apache#14583) fix: save non-parameter DBs (apache#14759) chore: Removes ColorSchemeControl.less (apache#14199) fix(explore): Icons width (apache#14717) ...
…e" (apache#14737) * fix(explore): DndColumnSelect not handling controls with multi={false} * Implement translations for singular and plural cases * Fix test
…e" (apache#14737) * fix(explore): DndColumnSelect not handling controls with multi={false} * Implement translations for singular and plural cases * Fix test
…e" (apache#14737) * fix(explore): DndColumnSelect not handling controls with multi={false} * Implement translations for singular and plural cases * Fix test
SUMMARY
DndColumnSelect
was working correctly only for controls that accept multiple values. When used with controls that hadmulti: false
prop, it displayed a prompt to add more columns and behaved weirdly - sometimes it allowed adding more values, sometimes it replaced the first value with whatever user dropped last. This PR fixes those glitches and unifies the behaviour with withDndMetricSelect
- after the first value is added, prompt disappears and adding new columns is disabled.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
118995851-c1518200-b987-11eb-99f0-b7adf922429c.mov
After:
118996113-f4941100-b987-11eb-8970-04bff622fe2c.mov
TEST PLAN
Open a chart that uses
DndColumnSelect
for single value controls (e.g. Country Map and "ISO 3166-2 CODES" control), verify that only 1 value can be added and there are no glitches when you try to add more values. Please don't mind type label overlapping with column name, the fix is ready in a different PR 🙂ADDITIONAL INFORMATION
CC: @villebro @junlincc