You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2024/03/28 14:32:07 UTC
(superset) branch master updated: fix: Select onChange is fired when the same item is selected in single mode (#27706)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new d69a1870a0 fix: Select onChange is fired when the same item is selected in single mode (#27706)
d69a1870a0 is described below
commit d69a1870a02787381345c7e67cbb1803d708b2f6
Author: Michael S. Molina <70...@users.noreply.github.com>
AuthorDate: Thu Mar 28 11:32:00 2024 -0300
fix: Select onChange is fired when the same item is selected in single mode (#27706)
---
.../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')),
);
}