feat: Select all for synchronous select #22084

cccs-RyanK · 2022-11-09T20:17:07Z

SUMMARY

This PR adds a 'select all' functionality to the synchronous Select component. This functionality has been requested by a large number of users and contributors. The select component has been previously split into sync and async components to simplify the code. The sync select component is the more widely used component, and a select all for the sync case is much easier to implement. This PR simply adds the option for selectAllEnabled only when the select component is in multi-select mode. The option is by default set to false. This is part of the greater task to rework the select component for the select all functionality outlined by @michael-s-molina in this PR: #20143

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

In storybook, go to the select component -> interactive select and set the mode to multi and selectAllEnabled to True

ADDITIONAL INFORMATION

Has associated issue: "Select All" functionality needed in column select #18247 (comment)
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

…elect-all-synchronous-select

michael-s-molina

Thanks for the PR @cccs-RyanK. I left some first-pass comments.

superset-frontend/src/components/Select/types.ts

superset-frontend/src/components/Select/utils.tsx

superset-frontend/src/components/Select/Select.test.tsx

superset-frontend/src/components/Select/utils.tsx

…elect-all-synchronous-select

cccs-RyanK · 2022-12-01T13:59:04Z

superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx

@@ -94,7 +94,7 @@ test('table should be visible when expanded is true', async () => {
    expect(dbSelect).toBeInTheDocument();
    expect(schemaSelect).toBeInTheDocument();
    expect(dropdown).toBeInTheDocument();
-    expect(abUser).toHaveLength(2);
+    expect(abUser).toHaveLength(1);


This test adds one item into a 'multi' select component and then selects it. With the new behaviour, that counts as selecting all which renders a custom tag with the total number of items selected. This test would fail because it expected the single tag to be rendered in the select component when that is no longer the expected behaviour, so I changed the test.

Another option is to make it so that the select all functionality is disabled in the case where there is only one item in the options. Please let me know your thoughts

I think I like the idea of disabling the "select all" when only 1 option is available.

codecov · 2022-12-01T14:40:03Z

Codecov Report

Merging #22084 (0d66466) into master (1a0de49) will decrease coverage by 0.10%.
The diff coverage is 85.33%.

@@            Coverage Diff             @@
##           master   #22084      +/-   ##
==========================================
- Coverage   67.09%   66.99%   -0.10%     
==========================================
  Files        1869     1876       +7     
  Lines       71597    71814     +217     
  Branches     7821     7879      +58     
==========================================
+ Hits        48035    48110      +75     
- Misses      21534    21675     +141     
- Partials     2028     2029       +1

Flag	Coverage Δ
javascript	`53.79% <85.33%> (-0.12%)`	⬇️

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

Impacted Files	Coverage Δ
...set-frontend/src/components/Select/AsyncSelect.tsx	`88.46% <ø> (-0.07%)`	⬇️
...c/filters/components/Select/SelectFilterPlugin.tsx	`62.82% <ø> (-0.48%)`	⬇️
superset-frontend/src/components/Select/styles.tsx	`80.76% <66.66%> (-1.84%)`	⬇️
superset-frontend/src/components/Select/Select.tsx	`91.61% <84.61%> (-1.53%)`	⬇️
...erset-frontend/src/components/Select/CustomTag.tsx	`75.00% <100.00%> (+3.57%)`	⬆️
superset-frontend/src/components/Select/utils.tsx	`82.14% <100.00%> (+0.66%)`	⬆️
superset-frontend/src/components/Modal/Modal.tsx	`85.89% <0.00%> (-3.30%)`	⬇️
...plugin-chart-echarts/src/Treemap/transformProps.ts	`50.87% <0.00%> (-1.37%)`	⬇️
superset-frontend/src/GlobalStyles.tsx	`0.00% <0.00%> (ø)`
.../plugins/plugin-chart-echarts/src/Treemap/types.ts	`100.00% <0.00%> (ø)`
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kgabryje · 2022-12-05T11:26:10Z

/testenv up

github-actions · 2022-12-05T11:28:10Z

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

EugeneTorap · 2022-12-05T11:46:55Z

Hi @cccs-RyanK. Thank you for this PR!
Can we try to use 'Select All' in columns, table viz? Or it'll be next PR?

michael-s-molina

Thanks for addressing the first-pass review comments @cccs-RyanK. Here are the second review comments:

When "Select all" is checked, we shouldn't show the +N tag.

We're showing "Select All (12)" but there are 11 items. We shouldn't count the "Select All" option:

superset-frontend/src/components/Select/Select.test.tsx

superset-frontend/src/components/Select/Select.tsx

michael-s-molina · 2022-12-12T16:31:07Z

superset-frontend/src/components/Select/Select.tsx

    useEffect(() => {
-      setSelectValue(value);
+      // if all values are selected, add select all to value
+      if (


You're missing fullSelectOptions.length, isSingleMode, labelInValue in the dependencies.

added isSingleMode and labelInValue as dependencies, but i purposefully left out fullSelectOptions.length because this hook should only be called when the component props have been changed. fullSelectOptions can change when new options are added and in that case we don't want the select value to be reset (erasing the newly added options).

Good point. This is indicative that the effect should be broken into two unrelated operations. Like this:

useEffect(() => { setSelectValue(value); }, [value]); useEffect(() => { // if all values are selected, add select all to value if ( !isSingleMode && ensureIsArray(value).length === fullSelectOptions.length && fullSelectOptions.length > 0 ) { setSelectValue( labelInValue ? ([...ensureIsArray(value), selectAllOption] as AntdLabeledValue[]) : ([ ...ensureIsArray(value), SELECT_ALL_VALUE, ] as AntdLabeledValue[]), ); } }, [value, isSingleMode, labelInValue, fullSelectOptions.length]);

michael-s-molina · 2022-12-12T17:16:38Z

When allowNewOptions is enabled, if you add a new option and remove it, "Select All" will be unchecked.

Screen.Recording.2022-12-12.at.12.13.43.PM.mov

michael-s-molina · 2022-12-12T17:24:49Z

When fixing the reported issues, can you please add the following test cases?

+N tag does not count the "Select All" option
does not show +N tag when "Select All" is checked
"Select All" is checked when unchecking an added option and all the other options are selected

villebro · 2023-01-06T09:25:36Z

FYI @cccs-RyanK @EugeneTorap this is on my priority todo list, and I'm hoping to give it a proper review+test pass this weekend.

michael-s-molina · 2023-01-11T14:48:43Z

Thanks for the feedback @geido ! Made the suggested changes from your first two points. As for the last point you made about the new options being erased when they are deselected, I do think that this behaviour is consistent with how the component currently works. For example, before the select-all changes if you add new values by themselves and then deselect them, they are erased from the options. My thoughts are that deselecting-all should then also remove all the new options. Please let me know if that makes sense to you.

@cccs-RyanK @geido I think we should disassociate the uncheck and remove actions on new items. We could have an X icon to remove new items. I can think of a scenario where a user is adding filters to a chart and after seeing the results, the user can check/uncheck the added filters to test different combinations. Making the remove action more intentional will improve UX. Let's tackle this in a separate PR. For now, I'm fine with removing all items when Select All is unchecked.

michael-s-molina · 2023-01-11T16:32:36Z

When working in oneLine mode, it's counting the Select All option.

Screen.Recording.2023-01-11.at.11.30.18.AM.mov

michael-s-molina · 2023-01-11T23:07:57Z

superset-frontend/src/components/Select/CustomTag.tsx

-  );
+  if (value !== SELECT_ALL_VALUE) {
+    return (
+      <Tag onMouseDown={onPreventMouseDown} {...props}>


To get rid of the typescript error:

Suggested change

<Tag onMouseDown={onPreventMouseDown} {...props}>

<Tag onMouseDown={onPreventMouseDown} {...(props as object)}>

michael-s-molina · 2023-01-11T23:14:32Z

superset-frontend/src/components/Select/AsyncSelect.tsx

@@ -527,7 +527,7 @@ const AsyncSelect = forwardRef(
            )
          }
          oneLine={oneLine}
-          tagRender={oneLine ? oneLineTagRender : undefined}
+          tagRender={customTagRender}


Previously, we only used a custom tag renderer if oneLine was true. Does customTagRender preserve the same behavior?

yes customTagRender is the same as oneLineTagRender. It has the same behaviour except it does not render the select all tag.

What I mean to ask is if customTagRender has the same behavior as tagRender={undefined}

Oh I see. The only difference between customTagRender and the default is the inclusion of the onMouseDown handler that prevents the dropdown from opening when clicking the tag icons. This was added for the oneLine mode and it seemed useful for the regular use case, but i can add a check to not include it when the component is not in oneLine mode?

Interesting. I like the behavior. No change is needed then.

…elect-all-synchronous-select

michael-s-molina

LGTM. Thank you for all the hard work and for addressing all the comments. This feature is a nice addition to the component!

rusackas

Not sure why @michael-s-molina 's review didn't light up the merge button. Adding another approval to see if it unblocks.

github-actions · 2023-01-18T12:42:23Z

Ephemeral environment shutdown and build artifacts deleted.

john-bodley · 2023-05-18T16:59:19Z

@cccs-RyanK and @michael-s-molina I was wondering whether this feature could be configurable, especially as it pertains to SQL Lab. For example at Airbnb we have namespaces which house thousands of tables/views (see attachment) and if the user accidentally chooses the Select All option` SQL Lab will implode.

It's unclear if this option really has any value in the context of SQL Lab, i.e., I'm struggling to foresee a scenario when one would ever needs to see the schema for all the tables/views within a namespace as opposed to selecting the subset to entities they're interested in.

michael-s-molina · 2023-05-18T17:06:54Z

I think it makes sense to disable the Select All option in that context.

villebro · 2023-05-18T17:14:57Z

I agree with @michael-s-molina , let's disable it in that particular context.

cccs-RyanK added 4 commits October 19, 2022 08:48

added select all button and a few tests

0dfab9d

Merge branch 'master' of github.com:CybercentreCanada/superset into s…

32e966d

…elect-all-synchronous-select

removed commented tests

66af8c9

prettier fixes

6e7ac16

cccs-RyanK requested review from michael-s-molina, geido and ktmud as code owners November 9, 2022 20:17

pull-request-size bot added the size/M label Nov 9, 2022

lint fixes

4c599fd

michael-s-molina reviewed Nov 10, 2022

View reviewed changes

added tests and feature for custom tag renderer

06b77b0

pull-request-size bot added size/L and removed size/M labels Nov 18, 2022

cccs-RyanK added 8 commits November 23, 2022 09:01

fixed onchange

1efc58b

Merge branch 'master' of github.com:CybercentreCanada/superset into s…

42b45fd

…elect-all-synchronous-select

fixed value format

14129d0

Merge branch 'master' of github.com:CybercentreCanada/superset into s…

3b596c6

…elect-all-synchronous-select

fixing wrong type issues

16e3c66

select test fixes

c69b7af

SqlEditor test changed with new select all behavior

3d120d1

fixed some lint errors

6ca115e

cccs-RyanK commented Dec 1, 2022

View reviewed changes

cccs-RyanK requested a review from michael-s-molina December 1, 2022 14:00

michael-s-molina reviewed Dec 12, 2022

View reviewed changes

michael-s-molina reviewed Jan 11, 2023

View reviewed changes

GITHUB_USERNAME added 5 commits January 12, 2023 15:59

fixed hook dependencies and ts error

88f125e

Merge branch 'master' of github.com:CybercentreCanada/superset into s…

807f51c

…elect-all-synchronous-select

removed custom max tag placeholder from select filter plugin

618161f

prettier fixes

6b9b428

fixed bug in maxTagPlaceholder and value filter plugin tests

0d66466

cccs-RyanK requested review from jinghua-qa, eschutho, rusackas and betodealmeida as code owners January 17, 2023 14:11

michael-s-molina approved these changes Jan 17, 2023

View reviewed changes

rusackas approved these changes Jan 17, 2023

View reviewed changes

jinghua-qa approved these changes Jan 18, 2023

View reviewed changes

michael-s-molina merged commit 02c9242 into apache:master Jan 18, 2023

This was referenced Jan 20, 2023

fix: Unexpected error on simple filter #22814

Merged

"Select All" functionality needed in column select #19426

Open

fix: Handles disabled options on Select All #22830

Merged

This was referenced May 18, 2023

fix(sqllab): Disable Select All on table selector #24120

Merged

fix(sqllab): Table options rendering regression #24142

Merged

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Select all for synchronous select #22084

feat: Select all for synchronous select #22084

cccs-RyanK commented Nov 9, 2022

michael-s-molina left a comment

cccs-RyanK Dec 1, 2022

geido Dec 13, 2022

codecov bot commented Dec 1, 2022 •

edited

Loading

kgabryje commented Dec 5, 2022

github-actions bot commented Dec 5, 2022

EugeneTorap commented Dec 5, 2022

michael-s-molina left a comment

michael-s-molina Dec 12, 2022

cccs-RyanK Dec 20, 2022

michael-s-molina Jan 11, 2023

michael-s-molina commented Dec 12, 2022

michael-s-molina commented Dec 12, 2022

villebro commented Jan 6, 2023

michael-s-molina commented Jan 11, 2023

michael-s-molina commented Jan 11, 2023

michael-s-molina Jan 11, 2023

michael-s-molina Jan 11, 2023

cccs-RyanK Jan 12, 2023

michael-s-molina Jan 12, 2023

cccs-RyanK Jan 13, 2023 •

edited

Loading

michael-s-molina Jan 13, 2023

michael-s-molina left a comment

rusackas left a comment

github-actions bot commented Jan 18, 2023

john-bodley commented May 18, 2023 •

edited

Loading

michael-s-molina commented May 18, 2023

villebro commented May 18, 2023

	<Tag onMouseDown={onPreventMouseDown} {...props}>
	<Tag onMouseDown={onPreventMouseDown} {...(props as object)}>

feat: Select all for synchronous select #22084

feat: Select all for synchronous select #22084

Conversation

cccs-RyanK commented Nov 9, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

michael-s-molina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 1, 2022 • edited Loading

Codecov Report

kgabryje commented Dec 5, 2022

github-actions bot commented Dec 5, 2022

EugeneTorap commented Dec 5, 2022

michael-s-molina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina commented Dec 12, 2022

michael-s-molina commented Dec 12, 2022

villebro commented Jan 6, 2023

michael-s-molina commented Jan 11, 2023

michael-s-molina commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cccs-RyanK Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2023

john-bodley commented May 18, 2023 • edited Loading

michael-s-molina commented May 18, 2023

villebro commented May 18, 2023

codecov bot commented Dec 1, 2022 •

edited

Loading

cccs-RyanK Jan 13, 2023 •

edited

Loading

john-bodley commented May 18, 2023 •

edited

Loading