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 2023/07/24 16:04:00 UTC
[superset] branch master updated: fix: Select onChange is being fired without explicit selection (#24698)
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 6089b5fdae fix: Select onChange is being fired without explicit selection (#24698)
6089b5fdae is described below
commit 6089b5fdaee7f0076d8e4c4a531e1b125b3f1010
Author: Michael S. Molina <70...@users.noreply.github.com>
AuthorDate: Mon Jul 24 13:03:45 2023 -0300
fix: Select onChange is being fired without explicit selection (#24698)
Co-authored-by: JUST.in DO IT <ju...@airbnb.com>
---
.../src/components/ListView/ListView.test.jsx | 2 +-
.../src/components/Select/AsyncSelect.test.tsx | 23 ++-
.../src/components/Select/AsyncSelect.tsx | 76 ++++++--
.../src/components/Select/Select.stories.tsx | 2 +
.../src/components/Select/Select.test.tsx | 19 ++
superset-frontend/src/components/Select/Select.tsx | 202 +++++++++++++++------
superset-frontend/src/components/Select/types.ts | 1 +
superset-frontend/src/components/Select/utils.tsx | 4 +-
.../src/features/alerts/AlertReportModal.test.jsx | 2 +-
9 files changed, 253 insertions(+), 78 deletions(-)
diff --git a/superset-frontend/src/components/ListView/ListView.test.jsx b/superset-frontend/src/components/ListView/ListView.test.jsx
index 25ab1b63ae..3135d4a6f5 100644
--- a/superset-frontend/src/components/ListView/ListView.test.jsx
+++ b/superset-frontend/src/components/ListView/ListView.test.jsx
@@ -470,7 +470,7 @@ describe('ListView', () => {
});
await act(async () => {
- wrapper2.find('[aria-label="Sort"]').first().props().onChange({
+ wrapper2.find('[aria-label="Sort"]').first().props().onSelect({
desc: false,
id: 'something',
label: 'Alphabetical',
diff --git a/superset-frontend/src/components/Select/AsyncSelect.test.tsx b/superset-frontend/src/components/Select/AsyncSelect.test.tsx
index ac9c78767c..0b7cafab3a 100644
--- a/superset-frontend/src/components/Select/AsyncSelect.test.tsx
+++ b/superset-frontend/src/components/Select/AsyncSelect.test.tsx
@@ -522,7 +522,7 @@ test('changes the selected item in single mode', async () => {
label: firstOption.label,
value: firstOption.value,
}),
- firstOption,
+ expect.objectContaining(firstOption),
);
expect(await findSelectValue()).toHaveTextContent(firstOption.label);
userEvent.click(await findSelectOption(secondOption.label));
@@ -531,7 +531,7 @@ test('changes the selected item in single mode', async () => {
label: secondOption.label,
value: secondOption.value,
}),
- secondOption,
+ expect.objectContaining(secondOption),
);
expect(await findSelectValue()).toHaveTextContent(secondOption.label);
});
@@ -804,6 +804,25 @@ test('Renders only an overflow tag if dropdown is open in oneLine mode', async (
expect(withinSelector.getByText('+ 2 ...')).toBeVisible();
});
+test('does not fire onChange when searching but no selection', async () => {
+ const onChange = jest.fn();
+ render(
+ <div role="main">
+ <AsyncSelect
+ {...defaultProps}
+ onChange={onChange}
+ mode="multiple"
+ allowNewOptions
+ />
+ </div>,
+ );
+ await open();
+ await type('Joh');
+ userEvent.click(await findSelectOption('John'));
+ userEvent.click(screen.getByRole('main'));
+ 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 ca9a6ee078..3453ce2b5f 100644
--- a/superset-frontend/src/components/Select/AsyncSelect.tsx
+++ b/superset-frontend/src/components/Select/AsyncSelect.tsx
@@ -28,7 +28,7 @@ import React, {
useCallback,
useImperativeHandle,
} from 'react';
-import { ensureIsArray, t } from '@superset-ui/core';
+import { ensureIsArray, t, usePrevious } from '@superset-ui/core';
import { LabeledValue as AntdLabeledValue } from 'antd/lib/select';
import debounce from 'lodash/debounce';
import { isEqual } from 'lodash';
@@ -47,6 +47,7 @@ import {
getSuffixIcon,
dropDownRenderHelper,
handleFilterOptionHelper,
+ mapOptions,
} from './utils';
import {
AsyncSelectProps,
@@ -54,6 +55,7 @@ import {
SelectOptionsPagePromise,
SelectOptionsType,
SelectOptionsTypePage,
+ SelectProps,
} from './types';
import {
StyledCheckOutlined,
@@ -102,6 +104,7 @@ const AsyncSelect = forwardRef(
allowClear,
allowNewOptions = false,
ariaLabel,
+ autoClearSearchValue = false,
fetchOnlyOnSearch,
filterOption = true,
header = null,
@@ -113,10 +116,13 @@ const AsyncSelect = forwardRef(
mode = 'single',
name,
notFoundContent,
+ onBlur,
onError,
onChange,
onClear,
onDropdownVisibleChange,
+ onDeselect,
+ onSelect,
optionFilterProps = ['label', 'value'],
options,
pageSize = DEFAULT_PAGE_SIZE,
@@ -150,10 +156,16 @@ const AsyncSelect = forwardRef(
? 'tags'
: 'multiple';
const allowFetch = !fetchOnlyOnSearch || inputValue;
-
const [maxTagCount, setMaxTagCount] = useState(
propsMaxTagCount ?? MAX_TAG_COUNT,
);
+ const [onChangeCount, setOnChangeCount] = useState(0);
+ const previousChangeCount = usePrevious(onChangeCount, 0);
+
+ const fireOnChange = useCallback(
+ () => setOnChangeCount(onChangeCount + 1),
+ [onChangeCount],
+ );
useEffect(() => {
if (oneLine) {
@@ -209,9 +221,7 @@ const AsyncSelect = forwardRef(
: selectOptions;
}, [selectOptions, selectValue]);
- const handleOnSelect = (
- selectedItem: string | number | AntdLabeledValue | undefined,
- ) => {
+ const handleOnSelect: SelectProps['onSelect'] = (selectedItem, option) => {
if (isSingleMode) {
setSelectValue(selectedItem);
} else {
@@ -228,12 +238,11 @@ const AsyncSelect = forwardRef(
return previousState;
});
}
- setInputValue('');
+ fireOnChange();
+ onSelect?.(selectedItem, option);
};
- const handleOnDeselect = (
- value: string | number | AntdLabeledValue | undefined,
- ) => {
+ const handleOnDeselect: SelectProps['onDeselect'] = (value, option) => {
if (Array.isArray(selectValue)) {
if (isLabeledValue(value)) {
const array = selectValue as AntdLabeledValue[];
@@ -245,7 +254,8 @@ const AsyncSelect = forwardRef(
setSelectValue(array.filter(element => element !== value));
}
}
- setInputValue('');
+ fireOnChange();
+ onDeselect?.(value, option);
};
const internalOnError = useCallback(
@@ -425,8 +435,50 @@ const AsyncSelect = forwardRef(
if (onClear) {
onClear();
}
+ fireOnChange();
+ };
+
+ const handleOnBlur = (event: React.FocusEvent<HTMLElement>) => {
+ const tagsMode = !isSingleMode && allowNewOptions;
+ const searchValue = inputValue.trim();
+ // Searched values will be autoselected during onBlur events when in tags mode.
+ // We want to make sure a value is only selected if the user has actually selected it
+ // by pressing Enter or clicking on it.
+ if (
+ tagsMode &&
+ searchValue &&
+ !hasOption(searchValue, selectValue, true)
+ ) {
+ // The search value will be added so we revert to the previous value
+ setSelectValue(selectValue || []);
+ }
+ onBlur?.(event);
};
+ useEffect(() => {
+ if (onChangeCount !== previousChangeCount) {
+ const array = ensureIsArray(selectValue);
+ const set = new Set(array.map(getValue));
+ const options = mapOptions(
+ fullSelectOptions.filter(opt => set.has(opt.value)),
+ );
+ if (isSingleMode) {
+ // @ts-ignore
+ onChange?.(selectValue, options[0]);
+ } else {
+ // @ts-ignore
+ onChange?.(array, options);
+ }
+ }
+ }, [
+ fullSelectOptions,
+ isSingleMode,
+ onChange,
+ onChangeCount,
+ previousChangeCount,
+ selectValue,
+ ]);
+
useEffect(() => {
// when `options` list is updated from component prop, reset states
fetchedQueries.current.clear();
@@ -482,7 +534,7 @@ const AsyncSelect = forwardRef(
<StyledSelect
allowClear={!isLoading && allowClear}
aria-label={ariaLabel || name}
- autoClearSearchValue={false}
+ autoClearSearchValue={autoClearSearchValue}
dropdownRender={dropdownRender}
filterOption={handleFilterOption}
filterSort={sortComparatorWithSearch}
@@ -494,13 +546,13 @@ const AsyncSelect = forwardRef(
maxTagCount={maxTagCount}
mode={mappedMode}
notFoundContent={isLoading ? t('Loading...') : notFoundContent}
+ onBlur={handleOnBlur}
onDeselect={handleOnDeselect}
onDropdownVisibleChange={handleOnDropdownVisibleChange}
onPopupScroll={handlePagination}
onSearch={showSearch ? handleOnSearch : undefined}
onSelect={handleOnSelect}
onClear={handleClear}
- onChange={onChange}
options={
hasCustomLabels(fullSelectOptions) ? undefined : fullSelectOptions
}
diff --git a/superset-frontend/src/components/Select/Select.stories.tsx b/superset-frontend/src/components/Select/Select.stories.tsx
index cb7224fdca..58fa424700 100644
--- a/superset-frontend/src/components/Select/Select.stories.tsx
+++ b/superset-frontend/src/components/Select/Select.stories.tsx
@@ -208,6 +208,8 @@ InteractiveSelect.args = {
autoFocus: true,
allowNewOptions: false,
allowClear: false,
+ autoClearSearchValue: false,
+ allowSelectAll: true,
showSearch: true,
disabled: false,
invertSelection: false,
diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx
index a5e596bd2f..bce753604f 100644
--- a/superset-frontend/src/components/Select/Select.test.tsx
+++ b/superset-frontend/src/components/Select/Select.test.tsx
@@ -890,6 +890,25 @@ test('"Select All" does not affect disabled options', async () => {
expect(await findSelectValue()).not.toHaveTextContent(options[1].label);
});
+test('does not fire onChange when searching but no selection', async () => {
+ const onChange = jest.fn();
+ render(
+ <div role="main">
+ <Select
+ {...defaultProps}
+ onChange={onChange}
+ mode="multiple"
+ allowNewOptions
+ />
+ </div>,
+ );
+ await open();
+ await type('Joh');
+ userEvent.click(await findSelectOption('John'));
+ userEvent.click(screen.getByRole('main'));
+ 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 1cc222f002..d9b022224c 100644
--- a/superset-frontend/src/components/Select/Select.tsx
+++ b/superset-frontend/src/components/Select/Select.tsx
@@ -30,6 +30,7 @@ import {
formatNumber,
NumberFormats,
t,
+ usePrevious,
} from '@superset-ui/core';
import AntdSelect, { LabeledValue as AntdLabeledValue } from 'antd/lib/select';
import { isEqual } from 'lodash';
@@ -86,6 +87,7 @@ const Select = forwardRef(
allowNewOptions = false,
allowSelectAll = true,
ariaLabel,
+ autoClearSearchValue = false,
filterOption = true,
header = null,
headerPosition = 'top',
@@ -96,9 +98,12 @@ const Select = forwardRef(
mode = 'single',
name,
notFoundContent,
+ onBlur,
onChange,
onClear,
onDropdownVisibleChange,
+ onDeselect,
+ onSelect,
optionFilterProps = ['label', 'value'],
options,
placeholder = t('Select ...'),
@@ -122,6 +127,13 @@ const Select = forwardRef(
const [maxTagCount, setMaxTagCount] = useState(
propsMaxTagCount ?? MAX_TAG_COUNT,
);
+ const [onChangeCount, setOnChangeCount] = useState(0);
+ const previousChangeCount = usePrevious(onChangeCount, 0);
+
+ const fireOnChange = useCallback(
+ () => setOnChangeCount(onChangeCount + 1),
+ [onChangeCount],
+ );
useEffect(() => {
if (oneLine) {
@@ -215,9 +227,7 @@ const Select = forwardRef(
[selectValue, selectAllEligible],
);
- const handleOnSelect = (
- selectedItem: string | number | AntdLabeledValue | undefined,
- ) => {
+ const handleOnSelect: SelectProps['onSelect'] = (selectedItem, option) => {
if (isSingleMode) {
setSelectValue(selectedItem);
} else {
@@ -252,26 +262,30 @@ const Select = forwardRef(
return previousState;
});
}
- setInputValue('');
+ fireOnChange();
+ onSelect?.(selectedItem, option);
};
const clear = () => {
- setSelectValue(
- fullSelectOptions
- .filter(
- option => option.disabled && hasOption(option.value, selectValue),
- )
- .map(option =>
- labelInValue
- ? { label: option.label, value: option.value }
- : option.value,
- ),
- );
+ if (isSingleMode) {
+ setSelectValue(undefined);
+ } else {
+ setSelectValue(
+ fullSelectOptions
+ .filter(
+ option => option.disabled && hasOption(option.value, selectValue),
+ )
+ .map(option =>
+ labelInValue
+ ? { label: option.label, value: option.value }
+ : option.value,
+ ),
+ );
+ fireOnChange();
+ }
};
- const handleOnDeselect = (
- value: string | number | AntdLabeledValue | undefined,
- ) => {
+ const handleOnDeselect: SelectProps['onDeselect'] = (value, option) => {
if (Array.isArray(selectValue)) {
if (getValue(value) === getValue(SELECT_ALL_VALUE)) {
clear();
@@ -292,7 +306,8 @@ const Select = forwardRef(
setSelectValue(array);
}
}
- setInputValue('');
+ fireOnChange();
+ onDeselect?.(value, option);
};
const handleOnSearch = (search: string) => {
@@ -312,7 +327,7 @@ const Select = forwardRef(
: cleanSelectOptions;
setSelectOptions(newOptions);
}
- setInputValue(search);
+ setInputValue(searchValue);
};
const handleFilterOption = (search: string, option: AntdLabeledValue) =>
@@ -393,8 +408,15 @@ const Select = forwardRef(
);
optionsToSelect.push(labelInValue ? selectAllOption : SELECT_ALL_VALUE);
setSelectValue(optionsToSelect);
+ fireOnChange();
}
- }, [selectValue, selectAllMode, labelInValue, selectAllEligible]);
+ }, [
+ selectValue,
+ selectAllMode,
+ labelInValue,
+ selectAllEligible,
+ fireOnChange,
+ ]);
const selectAllLabel = useMemo(
() => () =>
@@ -406,53 +428,115 @@ const Select = forwardRef(
[selectAllEligible],
);
- const handleOnChange = (values: any, options: any) => {
- // intercept onChange call to handle the select all case
- // if the "select all" option is selected, we want to send all options to the onChange,
- // otherwise we want to remove
- let newValues = values;
- let newOptions = options;
- if (!isSingleMode) {
- if (
- ensureIsArray(newValues).some(
- val => getValue(val) === SELECT_ALL_VALUE,
- )
- ) {
- // send all options to onchange if all are not currently there
- if (!selectAllMode) {
- newValues = mapValues(selectAllEligible, labelInValue);
- newOptions = mapOptions(selectAllEligible);
- } else {
- newValues = ensureIsArray(values).filter(
- (val: any) => getValue(val) !== SELECT_ALL_VALUE,
+ const handleOnBlur = (event: React.FocusEvent<HTMLElement>) => {
+ const tagsMode = !isSingleMode && allowNewOptions;
+ const searchValue = inputValue.trim();
+ // Searched values will be autoselected during onBlur events when in tags mode.
+ // We want to make sure a value is only selected if the user has actually selected it
+ // by pressing Enter or clicking on it.
+ if (
+ tagsMode &&
+ searchValue &&
+ !hasOption(searchValue, selectValue, true)
+ ) {
+ // The search value will be added so we revert to the previous value
+ setSelectValue(selectValue || []);
+ }
+ onBlur?.(event);
+ };
+
+ const handleOnChange = useCallback(
+ (values: any, options: any) => {
+ // intercept onChange call to handle the select all case
+ // if the "select all" option is selected, we want to send all options to the onChange,
+ // otherwise we want to remove
+ let newValues = values;
+ let newOptions = options;
+ if (!isSingleMode) {
+ if (
+ ensureIsArray(newValues).some(
+ val => getValue(val) === SELECT_ALL_VALUE,
+ )
+ ) {
+ // send all options to onchange if all are not currently there
+ if (!selectAllMode) {
+ newValues = mapValues(selectAllEligible, labelInValue);
+ newOptions = mapOptions(selectAllEligible);
+ } else {
+ newValues = ensureIsArray(values).filter(
+ (val: any) => getValue(val) !== SELECT_ALL_VALUE,
+ );
+ }
+ } else if (
+ ensureIsArray(values).length === selectAllEligible.length &&
+ selectAllMode
+ ) {
+ const array = selectAllEligible.filter(
+ option => hasOption(option.value, selectValue) && option.disabled,
);
+ newValues = mapValues(array, labelInValue);
+ newOptions = mapOptions(array);
}
- } else if (
- ensureIsArray(values).length === selectAllEligible.length &&
- selectAllMode
- ) {
- const array = selectAllEligible.filter(
- option => hasOption(option.value, selectValue) && option.disabled,
- );
- newValues = mapValues(array, labelInValue);
- newOptions = mapOptions(array);
+ }
+ onChange?.(newValues, newOptions);
+ },
+ [
+ isSingleMode,
+ labelInValue,
+ onChange,
+ selectAllEligible,
+ selectAllMode,
+ selectValue,
+ ],
+ );
+
+ useEffect(() => {
+ if (onChangeCount !== previousChangeCount) {
+ const array = ensureIsArray(selectValue);
+ const set = new Set(array.map(getValue));
+ const options = mapOptions(
+ fullSelectOptions.filter(opt => set.has(opt.value)),
+ );
+ if (isSingleMode) {
+ handleOnChange(selectValue, selectValue ? options[0] : undefined);
+ } else {
+ handleOnChange(array, options);
}
}
- onChange?.(newValues, newOptions);
- };
+ }, [
+ fullSelectOptions,
+ handleOnChange,
+ isSingleMode,
+ onChange,
+ onChangeCount,
+ previousChangeCount,
+ selectValue,
+ ]);
const shouldRenderChildrenOptions = useMemo(
() => selectAllEnabled || hasCustomLabels(options),
[selectAllEnabled, options],
);
- const customMaxTagPlaceholder = () => {
+ const omittedCount = useMemo(() => {
const num_selected = ensureIsArray(selectValue).length;
const num_shown = maxTagCount as number;
- return selectAllMode
- ? `+ ${num_selected - num_shown - 1} ...`
- : `+ ${num_selected - num_shown} ...`;
- };
+ return num_selected - num_shown - (selectAllMode ? 1 : 0);
+ }, [maxTagCount, selectAllMode, selectValue]);
+
+ const customMaxTagPlaceholder = () =>
+ `+ ${omittedCount > 0 ? omittedCount : 1} ...`;
+
+ // We can't remove the + tag so when Select All
+ // is the only item omitted, we subtract one from maxTagCount
+ let actualMaxTagCount = maxTagCount;
+ if (
+ actualMaxTagCount !== 'responsive' &&
+ omittedCount === 0 &&
+ selectAllMode
+ ) {
+ actualMaxTagCount -= 1;
+ }
return (
<StyledContainer headerPosition={headerPosition}>
@@ -462,7 +546,7 @@ const Select = forwardRef(
<StyledSelect
allowClear={!isLoading && allowClear}
aria-label={ariaLabel || name}
- autoClearSearchValue={false}
+ autoClearSearchValue={autoClearSearchValue}
dropdownRender={dropdownRender}
filterOption={handleFilterOption}
filterSort={sortComparatorWithSearch}
@@ -471,17 +555,17 @@ const Select = forwardRef(
}
headerPosition={headerPosition}
labelInValue={labelInValue}
- maxTagCount={maxTagCount}
+ maxTagCount={actualMaxTagCount}
maxTagPlaceholder={customMaxTagPlaceholder}
mode={mappedMode}
notFoundContent={isLoading ? t('Loading...') : notFoundContent}
+ onBlur={handleOnBlur}
onDeselect={handleOnDeselect}
onDropdownVisibleChange={handleOnDropdownVisibleChange}
onPopupScroll={undefined}
onSearch={shouldShowSearch ? handleOnSearch : undefined}
onSelect={handleOnSelect}
onClear={handleClear}
- onChange={handleOnChange}
placeholder={placeholder}
showSearch={shouldShowSearch}
showArrow
diff --git a/superset-frontend/src/components/Select/types.ts b/superset-frontend/src/components/Select/types.ts
index 6ab9d7478c..e8a1ee8248 100644
--- a/superset-frontend/src/components/Select/types.ts
+++ b/superset-frontend/src/components/Select/types.ts
@@ -40,6 +40,7 @@ export type AntdProps = AntdSelectProps<AntdSelectValue>;
export type AntdExposedProps = Pick<
AntdProps,
| 'allowClear'
+ | 'autoClearSearchValue'
| 'autoFocus'
| 'disabled'
| 'filterOption'
diff --git a/superset-frontend/src/components/Select/utils.tsx b/superset-frontend/src/components/Select/utils.tsx
index d6d352132f..7de201caeb 100644
--- a/superset-frontend/src/components/Select/utils.tsx
+++ b/superset-frontend/src/components/Select/utils.tsx
@@ -218,7 +218,5 @@ export const mapOptions = (values: SelectOptionsType) =>
values.map(opt => ({
children: opt.label,
key: opt.value,
- value: opt.value,
- label: opt.label,
- disabled: opt.disabled,
+ ...opt,
}));
diff --git a/superset-frontend/src/features/alerts/AlertReportModal.test.jsx b/superset-frontend/src/features/alerts/AlertReportModal.test.jsx
index e7fb99797a..a91e789920 100644
--- a/superset-frontend/src/features/alerts/AlertReportModal.test.jsx
+++ b/superset-frontend/src/features/alerts/AlertReportModal.test.jsx
@@ -341,7 +341,7 @@ describe('AlertReportModal', () => {
.find('[data-test="select-delivery-method"]')
.last()
.props()
- .onChange('Email');
+ .onSelect('Email');
});
await waitForComponentToPaint(wrapper);
expect(wrapper.find('textarea[name="recipients"]')).toHaveLength(1);