From 6089b5fdaee7f0076d8e4c4a531e1b125b3f1010 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Mon, 24 Jul 2023 13:03:45 -0300 Subject: [PATCH] fix: Select onChange is being fired without explicit selection (#24698) Co-authored-by: JUST.in DO IT --- .../src/components/ListView/ListView.test.jsx | 2 +- .../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 ++ .../src/components/Select/Select.tsx | 202 +++++++++++++----- .../src/components/Select/types.ts | 1 + .../src/components/Select/utils.tsx | 4 +- .../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 25ab1b63a..3135d4a6f 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 ac9c78767..0b7cafab3 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( +
+ +
, + ); + 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 ca9a6ee07..3453ce2b5 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) => { + 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( { expect(await findSelectValue()).not.toHaveTextContent(options[1].label); }); +test('does not fire onChange when searching but no selection', async () => { + const onChange = jest.fn(); + render( +
+