fix: Select onChange is fired when the same item is selected in singl…

…e mode (apache#27706)
EnxDev · Apr 12, 2024 · 2018727 · 2018727
1 parent 12bf3d0
commit 2018727
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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')),
   );
 }