iop
/
superset
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
This commit is contained in:
Michael S. Molina 2023-07-24 13:03:45 -03:00 committed by GitHub
parent b8a3eeffdb
commit 6089b5fdae
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 253 additions and 78 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
superset-frontend/src/components/ListView/ListView.test.jsx
View File

@ -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',

23
superset-frontend/src/components/Select/AsyncSelect.test.tsx
View File

@ -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

76
superset-frontend/src/components/Select/AsyncSelect.tsx
View File

@ -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
}

2
superset-frontend/src/components/Select/Select.stories.tsx
View File

@ -208,6 +208,8 @@ InteractiveSelect.args = {
autoFocus: true,
allowNewOptions: false,
allowClear: false,
autoClearSearchValue: false,
allowSelectAll: true,
showSearch: true,
disabled: false,
invertSelection: false,

19
superset-frontend/src/components/Select/Select.test.tsx
View File

@ -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

202
superset-frontend/src/components/Select/Select.tsx
View File

@ -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

1
superset-frontend/src/components/Select/types.ts
View File

@ -40,6 +40,7 @@ export type AntdProps = AntdSelectProps<AntdSelectValue>;
export type AntdExposedProps = Pick<
AntdProps,
| 'allowClear'
| 'autoClearSearchValue'
| 'autoFocus'
| 'disabled'
| 'filterOption'

4
superset-frontend/src/components/Select/utils.tsx
View File

@ -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,
}));

2
superset-frontend/src/features/alerts/AlertReportModal.test.jsx
View File

@ -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);