From bd248020c493253123c1b35c414eace6ea4a851c Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 18 Jul 2023 19:15:25 -0300 Subject: [PATCH 1/5] fix: Select onChange is being fired without explicit selection --- .../src/components/ListView/ListView.test.jsx | 2 +- .../components/Select/AsyncSelect.test.tsx | 23 ++- .../src/components/Select/AsyncSelect.tsx | 72 +++++++- .../src/components/Select/Select.test.tsx | 19 ++ .../src/components/Select/Select.tsx | 172 +++++++++++++----- .../src/components/Select/utils.tsx | 2 +- .../features/alerts/AlertReportModal.test.jsx | 2 +- 7 files changed, 229 insertions(+), 63 deletions(-) diff --git a/superset-frontend/src/components/ListView/ListView.test.jsx b/superset-frontend/src/components/ListView/ListView.test.jsx index 25ab1b63aea6b..3135d4a6f58b9 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 ac9c78767c4ae..0b7cafab3a141 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 ca9a6ee078a4d..b8871184b0873 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, @@ -113,10 +115,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 +155,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 +220,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 { @@ -229,11 +238,11 @@ const AsyncSelect = forwardRef( }); } 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[]; @@ -246,6 +255,8 @@ const AsyncSelect = forwardRef( } } setInputValue(''); + fireOnChange(); + onDeselect?.(value, option); }; const internalOnError = useCallback( @@ -425,8 +436,51 @@ 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 set = new Set(); + const array = ensureIsArray(selectValue); + array.forEach(item => set.add(getValue(item))); + 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(); @@ -494,13 +548,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.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx index a5e596bd2fdba..bce753604f511 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( +
+