fix: Select onChange is being fired without explicit selection #24698

michael-s-molina · 2023-07-14T16:44:25Z

SUMMARY

This PR fixes a bug in the Select component related to searched values that were automatically being selected without an explicit selection from the user. This happens because the onChange event is called when a value selection is made or when the input changes. This was only perceived with the introduction of autoClearSearchValue={false} in #23869. This PR changes the logic to only fire the onChange event when there's an actual selection by the user.

TESTING INSTRUCTIONS

I added a test to the component but we need to check the general use of selects in the application because there are wrappers that override the component behavior such as SelectControl and SelectFilterPlugin.

ADDITIONAL INFORMATION

Has associated issue:
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

michael-s-molina · 2023-07-18T22:34:51Z

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

+    const [onChangeCount, setOnChangeCount] = useState(0);
+    const previousChangeCount = usePrevious(onChangeCount, 0);
+
+    const fireOnChange = useCallback(


Simple counter used to fire the onChange event.

michael-s-molina · 2023-07-18T22:36:56Z

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

+          fullSelectOptions.filter(opt => set.has(opt.value)),
+        );
+        if (isSingleMode) {
+          // @ts-ignore


The original typescript definition does not allow undefined but I checked and the component allows it when using allowClear.

michael-s-molina · 2023-07-18T22:39:14Z

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

-              : option.value,
-          ),
-      );
+      if (isSingleMode) {


Single mode was previously using an empty array but it should be undefined.

michael-s-molina · 2023-07-18T22:42:03Z

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

-          newValues = mapValues(array, labelInValue);
-          newOptions = mapOptions(array);
+        }
+        onChange?.(newValues, newOptions);


No relevant changes to handleOnChange apart from adding useCallback.

michael-s-molina · 2023-07-18T22:43:30Z

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

          onDeselect={handleOnDeselect}
          onDropdownVisibleChange={handleOnDropdownVisibleChange}
          onPopupScroll={undefined}
          onSearch={shouldShowSearch ? handleOnSearch : undefined}
          onSelect={handleOnSelect}
          onClear={handleClear}
-          onChange={handleOnChange}


onChange is now manually fired when there's an actual change by the user.

michael-s-molina · 2023-07-18T22:44:38Z

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

    label: opt.label,
    disabled: opt.disabled,
+    ...opt,


Options may contain custom fields and these should be present when mapping.

michael-s-molina · 2023-07-18T22:46:53Z

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

+test('does not fire onChange when searching but no selection', async () => {
+  const onChange = jest.fn();
+  render(
+    <div role="main">


This div is to simulate a click outside of the select.

codecov · 2023-07-18T23:14:02Z

Codecov Report

Merging #24698 (14f2c7b) into master (cb9b865) will increase coverage by 8.88%.
The diff coverage is 74.31%.

❗ Current head 14f2c7b differs from pull request most recent head 6b06ea0. Consider uploading reports for the commit 6b06ea0 to get more accurate results

@@            Coverage Diff             @@
##           master   #24698      +/-   ##
==========================================
+ Coverage   58.35%   67.24%   +8.88%     
==========================================
  Files        1901     1902       +1     
  Lines       73933    73996      +63     
  Branches     8183     8195      +12     
==========================================
+ Hits        43146    49760    +6614     
+ Misses      28666    22117    -6549     
+ Partials     2121     2119       -2

Flag	Coverage Δ
javascript	`55.79% <74.12%> (+0.08%)`	⬆️
unit	`?`

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

Impacted Files	Coverage Δ
...src/components/ColumnTypeLabel/ColumnTypeLabel.tsx	`100.00% <ø> (ø)`
...d/packages/superset-ui-chart-controls/src/index.ts	`100.00% <ø> (ø)`
...-chart-controls/src/sections/advancedAnalytics.tsx	`14.28% <ø> (ø)`
...rset-ui-chart-controls/src/sections/chartTitle.tsx	`100.00% <ø> (ø)`
...legacy-plugin-chart-partition/src/controlPanel.tsx	`25.00% <ø> (ø)`
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx	`50.00% <ø> (ø)`
...gins/legacy-preset-chart-nvd3/src/NVD3Controls.tsx	`100.00% <ø> (ø)`
.../BigNumber/BigNumberWithTrendline/controlPanel.tsx	`16.66% <ø> (ø)`
...s/plugin-chart-echarts/src/Funnel/controlPanel.tsx	`66.66% <ø> (ø)`
...ns/plugin-chart-echarts/src/Gauge/controlPanel.tsx	`66.66% <ø> (ø)`
... and 45 more

... and 342 files with indirect coverage changes

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

michael-s-molina · 2023-07-19T14:28:59Z

@jinghua-qa @sadpandajoe Could you help us test the PR?

geido

Hey @michael-s-molina thanks for this. I see we are adding a bunch of logic to control how the onChange event is being triggered. Just trying to understand what's the benefit of doing this VS enforcing that the wrapper components use the onSelect / onDeselect handlers instead?

michael-s-molina · 2023-07-19T17:37:29Z

Hey @michael-s-molina thanks for this. I see we are adding a bunch of logic to control how the onChange event is being triggered. Just trying to understand what's the benefit of doing this VS enforcing that the wrapper components use the onSelect / onDeselect handlers instead?

That's a great question. The difference is that the onChange event works with all selected values at once which is ideal for bulk updates such as Select All.

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

Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>

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

geido · 2023-07-21T10:34:15Z

/testenv up

github-actions · 2023-07-21T10:38:08Z

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

geido · 2023-07-21T10:54:22Z

@michael-s-molina I think it makes sense to make the autoClearSearchValue an option that can be controlled from the wrappers. I am looking at the Dashboard properties to set owners. I don't think this functionality is very useful in that sense and it might be confusing as well. What do you think? Maybe we can consider that for a future iteration.

I found a small issue. When clicking on the input when there is a search value, the search value disappears. I think we need to control the target of the click.

FCC.New.Coder.Survey.2018.mp4

Another weird aspect of the Select, that has nothing to do with this PR, but that I'd like to point out. When hitting Enter on a new search value, I am expecting that the value will be added to the options, like it happens when there is no other match and the value is new. However, when the search value matches with an already selected value that is focused, hitting Enter not only does not select the search value, but deselects the focused value, which is super confusing.

FCC.New.Coder.Survey.2018.1.mp4

michael-s-molina · 2023-07-21T13:47:31Z

@michael-s-molina I think it makes sense to make the autoClearSearchValue an option that can be controlled from the wrappers. I am looking at the Dashboard properties to set owners. I don't think this functionality is very useful in that sense and it might be confusing as well. What do you think? Maybe we can consider that for a future iteration.

I exposed autoClearSearchValue as a property and now it can be overridden as you suggested.

I found a small issue. When clicking on the input when there is a search value, the search value disappears. I think we need to control the target of the click.

This is a bug with Ant Design that was fixed on version 4.24.5 which is some leap from our current 4.10.3 version. Unfortunately, we may delay this fix until Ant Design is upgraded.

michael-s-molina · 2023-07-21T14:34:43Z

Another weird aspect of the Select, that has nothing to do with this PR, but that I'd like to point out. When hitting Enter on a new search value, I am expecting that the value will be added to the options, like it happens when there is no other match and the value is new. However, when the search value matches with an already selected value that is focused, hitting Enter not only does not select the search value, but deselects the focused value, which is super confusing.

What's happening is that when you select an option, it gets focused and that's why when you press Enter, it gets unselected. If you use the arrow keys after selecting an item, you can see that if you focus a different option and press Enter it will get selected. So it's not related to the search value matching a selected option. One way we could improve this would be to always auto focus the next option after the selected ones but I didn't find a way to programmatically change the focus.

Screen.Recording.2023-07-21.at.11.27.51.mov

geido

Thanks for all the work on this!

github-actions · 2023-07-24T16:04:20Z

Ephemeral environment shutdown and build artifacts deleted.

Co-authored-by: JUST.in DO IT <justin.park@airbnb.com> (cherry picked from commit 6089b5f)

…e#24698) Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>

pull-request-size bot added the size/M label Jul 14, 2023

michael-s-molina force-pushed the fix-select-onchange branch from 10730fb to ad25283 Compare July 14, 2023 22:23

pull-request-size bot added size/L and removed size/M labels Jul 14, 2023

michael-s-molina force-pushed the fix-select-onchange branch from ad25283 to 9712800 Compare July 18, 2023 22:15

michael-s-molina requested a review from jinghua-qa July 18, 2023 22:33

michael-s-molina commented Jul 18, 2023

View reviewed changes

michael-s-molina marked this pull request as ready for review July 19, 2023 13:15

michael-s-molina requested review from geido and kgabryje as code owners July 19, 2023 13:15

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 19, 2023

geido reviewed Jul 19, 2023

View reviewed changes

fix: Select onChange is being fired without explicit selection

bd24802

michael-s-molina force-pushed the fix-select-onchange branch from 9712800 to bd24802 Compare July 20, 2023 14:35

apache deleted a comment from github-actions bot Jul 20, 2023

justinpark reviewed Jul 20, 2023

View reviewed changes

superset-frontend/src/components/Select/utils.tsx Outdated Show resolved Hide resolved

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

05c3aaa

Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>

justinpark reviewed Jul 20, 2023

View reviewed changes

superset-frontend/src/components/Select/AsyncSelect.tsx Outdated Show resolved Hide resolved

apache deleted a comment from github-actions bot Jul 20, 2023

Makes Set creation shorter

74161e4

michael-s-molina requested a review from geido July 20, 2023 19:31

Exposes autoClearSearchValue as a property

a88148d

Fixes tag count placeholder

6b06ea0

geido approved these changes Jul 24, 2023

View reviewed changes

michael-s-molina merged commit 6089b5f into apache:master Jul 24, 2023

michael-s-molina added a commit that referenced this pull request Jul 26, 2023

fix: Select onChange is being fired without explicit selection (#24698)

d336431

Co-authored-by: JUST.in DO IT <justin.park@airbnb.com> (cherry picked from commit 6089b5f)

mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix: Select onChange is being fired without explicit selection (apach…

35d21bb

…e#24698) Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Select onChange is being fired without explicit selection #24698

fix: Select onChange is being fired without explicit selection #24698

michael-s-molina commented Jul 14, 2023 •

edited

Loading

michael-s-molina Jul 18, 2023

michael-s-molina Jul 18, 2023

michael-s-molina Jul 18, 2023 •

edited

Loading

michael-s-molina Jul 18, 2023

michael-s-molina Jul 18, 2023

michael-s-molina Jul 18, 2023

michael-s-molina Jul 18, 2023

codecov bot commented Jul 18, 2023 •

edited

Loading

michael-s-molina commented Jul 19, 2023

geido left a comment

michael-s-molina commented Jul 19, 2023

geido commented Jul 21, 2023

github-actions bot commented Jul 21, 2023

geido commented Jul 21, 2023

michael-s-molina commented Jul 21, 2023 •

edited

Loading

michael-s-molina commented Jul 21, 2023

geido left a comment

github-actions bot commented Jul 24, 2023

fix: Select onChange is being fired without explicit selection #24698

fix: Select onChange is being fired without explicit selection #24698

Conversation

michael-s-molina commented Jul 14, 2023 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

michael-s-molina Jul 18, 2023

Choose a reason for hiding this comment

michael-s-molina Jul 18, 2023

Choose a reason for hiding this comment

michael-s-molina Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Jul 18, 2023

Choose a reason for hiding this comment

michael-s-molina Jul 18, 2023

Choose a reason for hiding this comment

michael-s-molina Jul 18, 2023

Choose a reason for hiding this comment

michael-s-molina Jul 18, 2023

Choose a reason for hiding this comment

codecov bot commented Jul 18, 2023 • edited Loading

Codecov Report

michael-s-molina commented Jul 19, 2023

geido left a comment

Choose a reason for hiding this comment

michael-s-molina commented Jul 19, 2023

geido commented Jul 21, 2023

github-actions bot commented Jul 21, 2023

geido commented Jul 21, 2023

michael-s-molina commented Jul 21, 2023 • edited Loading

michael-s-molina commented Jul 21, 2023

geido left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 24, 2023

michael-s-molina commented Jul 14, 2023 •

edited

Loading

michael-s-molina Jul 18, 2023 •

edited

Loading

codecov bot commented Jul 18, 2023 •

edited

Loading

michael-s-molina commented Jul 21, 2023 •

edited

Loading