This is an automated email from the ASF dual-hosted git repository. michaelsmolina pushed a commit to branch 4.0 in repository https://gitbox.apache.org/repos/asf/superset.git
commit b7fa3edcf41e92ae242d1f4162004248b67ba886 Author: Michael S. Molina <[email protected]> AuthorDate: Thu Mar 28 11:32:00 2024 -0300 fix: Select onChange is fired when the same item is selected in single mode (#27706) (cherry picked from commit d69a1870a02787381345c7e67cbb1803d708b2f6) --- .../src/components/Select/AsyncSelect.test.tsx | 12 ++++++++++++ .../src/components/Select/AsyncSelect.tsx | 13 ++++++++++++- .../src/components/Select/Select.test.tsx | 12 ++++++++++++ superset-frontend/src/components/Select/Select.tsx | 12 +++++++++++- superset-frontend/src/components/Select/utils.tsx | 18 ++++++++++-------- 5 files changed, 57 insertions(+), 10 deletions(-) diff --git a/superset-frontend/src/components/Select/AsyncSelect.test.tsx b/superset-frontend/src/components/Select/AsyncSelect.test.tsx index 4a2ba0007c..8e8e151b66 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.test.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.test.tsx @@ -939,6 +939,18 @@ test('pasting an existing option does not duplicate it in multiple mode', async expect(await findAllSelectOptions()).toHaveLength(4); }); +test('does not fire onChange if the same value is selected in single mode', async () => { + const onChange = jest.fn(); + render(<AsyncSelect {...defaultProps} onChange={onChange} />); + const optionText = 'Emma'; + await open(); + expect(onChange).toHaveBeenCalledTimes(0); + userEvent.click(await findSelectOption(optionText)); + expect(onChange).toHaveBeenCalledTimes(1); + userEvent.click(await findSelectOption(optionText)); + expect(onChange).toHaveBeenCalledTimes(1); +}); + /* TODO: Add tests that require scroll interaction. Needs further investigation. - Fetches more data when scrolling and more data is available diff --git a/superset-frontend/src/components/Select/AsyncSelect.tsx b/superset-frontend/src/components/Select/AsyncSelect.tsx index d41c87f047..3fb8bceaf4 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.tsx @@ -50,10 +50,12 @@ import { mapOptions, getOption, isObject, + isEqual as utilsIsEqual, } from './utils'; import { AsyncSelectProps, AsyncSelectRef, + RawValue, SelectOptionsPagePromise, SelectOptionsType, SelectOptionsTypePage, @@ -220,7 +222,16 @@ const AsyncSelect = forwardRef( const handleOnSelect: SelectProps['onSelect'] = (selectedItem, option) => { if (isSingleMode) { + // on select is fired in single value mode if the same value is selected + const valueChanged = !utilsIsEqual( + selectedItem, + selectValue as RawValue | AntdLabeledValue, + 'value', + ); setSelectValue(selectedItem); + if (valueChanged) { + fireOnChange(); + } } else { setSelectValue(previousState => { const array = ensureIsArray(previousState); @@ -234,8 +245,8 @@ const AsyncSelect = forwardRef( } return previousState; }); + fireOnChange(); } - fireOnChange(); onSelect?.(selectedItem, option); }; diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx index 2910353295..1daff06d4d 100644 --- a/superset-frontend/src/components/Select/Select.test.tsx +++ b/superset-frontend/src/components/Select/Select.test.tsx @@ -1053,6 +1053,18 @@ test('pasting an existing option does not duplicate it in multiple mode', async expect(await findAllSelectOptions()).toHaveLength(4); }); +test('does not fire onChange if the same value is selected in single mode', async () => { + const onChange = jest.fn(); + render(<Select {...defaultProps} onChange={onChange} />); + const optionText = 'Emma'; + await open(); + expect(onChange).toHaveBeenCalledTimes(0); + userEvent.click(await findSelectOption(optionText)); + expect(onChange).toHaveBeenCalledTimes(1); + userEvent.click(await findSelectOption(optionText)); + expect(onChange).toHaveBeenCalledTimes(1); +}); + /* TODO: Add tests that require scroll interaction. Needs further investigation. - Fetches more data when scrolling and more data is available diff --git a/superset-frontend/src/components/Select/Select.tsx b/superset-frontend/src/components/Select/Select.tsx index f4f9565abb..3db455cbe2 100644 --- a/superset-frontend/src/components/Select/Select.tsx +++ b/superset-frontend/src/components/Select/Select.tsx @@ -53,6 +53,7 @@ import { hasCustomLabels, getOption, isObject, + isEqual as utilsIsEqual, } from './utils'; import { RawValue, SelectOptionsType, SelectProps } from './types'; import { @@ -227,7 +228,16 @@ const Select = forwardRef( const handleOnSelect: SelectProps['onSelect'] = (selectedItem, option) => { if (isSingleMode) { + // on select is fired in single value mode if the same value is selected + const valueChanged = !utilsIsEqual( + selectedItem, + selectValue as RawValue | AntdLabeledValue, + 'value', + ); setSelectValue(selectedItem); + if (valueChanged) { + fireOnChange(); + } } else { setSelectValue(previousState => { const array = ensureIsArray(previousState); @@ -259,8 +269,8 @@ const Select = forwardRef( } return previousState; }); + fireOnChange(); } - fireOnChange(); onSelect?.(selectedItem, option); }; diff --git a/superset-frontend/src/components/Select/utils.tsx b/superset-frontend/src/components/Select/utils.tsx index 0b638f4f01..67b2a0191b 100644 --- a/superset-frontend/src/components/Select/utils.tsx +++ b/superset-frontend/src/components/Select/utils.tsx @@ -49,22 +49,24 @@ export function getValue( return isLabeledValue(option) ? option.value : option; } +export function isEqual(a: V | LabeledValue, b: V | LabeledValue, key: string) { + const actualA = isObject(a) && key in a ? a[key] : a; + const actualB = isObject(b) && key in b ? b[key] : b; + // When comparing the values we use the equality + // operator to automatically convert different types + // eslint-disable-next-line eqeqeq + return actualA == actualB; +} + export function getOption( value: V, options?: V | LabeledValue | (V | LabeledValue)[], checkLabel = false, ): V | LabeledValue { const optionsArray = ensureIsArray(options); - // When comparing the values we use the equality - // operator to automatically convert different types return optionsArray.find( x => - // eslint-disable-next-line eqeqeq - x == value || - (isObject(x) && - // eslint-disable-next-line eqeqeq - (('value' in x && x.value == value) || - (checkLabel && 'label' in x && x.label === value))), + isEqual(x, value, 'value') || (checkLabel && isEqual(x, value, 'label')), ); }
