From fe919741632e677ff14cdd35f886c8b2a9f0d4de Mon Sep 17 00:00:00 2001 From: cccs-RyanK <102618419+cccs-RyanK@users.noreply.github.com> Date: Thu, 28 Jul 2022 09:09:37 -0400 Subject: [PATCH] chore: Remove unecessary code from async and sync select components (#20690) * Created AsyncSelect Component Changed files to reference AsyncSelect if needed * modified import of AsyncSelect, removed async tests and prefixes from select tests * fixed various import and lint warnings * fixing lint errors * fixed frontend test errors * fixed alertreportmodel tests * removed accidental import * fixed lint errors * updated async select * removed code from select component * fixed select test * fixed async label value and select initial values * cleaned up async test * fixed lint errors * minor fixes to sync select component * removed unecessary variables and fixed linting * fixed npm test errors * fixed linting issues * fixed showSearch and storybook * fixed linting --- .../src/components/DatabaseSelector/index.tsx | 1 - .../components/ListView/Filters/Select.tsx | 42 ++- .../components/Select/AsyncSelect.test.tsx | 150 +++++++--- .../src/components/Select/AsyncSelect.tsx | 73 ++--- .../src/components/Select/Select.stories.tsx | 28 +- .../src/components/Select/Select.test.tsx | 27 +- .../src/components/Select/Select.tsx | 280 +----------------- .../src/components/TableSelector/index.tsx | 1 - .../src/filters/components/GroupBy/types.ts | 3 +- .../filters/components/TimeColumn/types.ts | 3 +- .../src/filters/components/TimeGrain/types.ts | 3 +- 11 files changed, 214 insertions(+), 397 deletions(-) diff --git a/superset-frontend/src/components/DatabaseSelector/index.tsx b/superset-frontend/src/components/DatabaseSelector/index.tsx index e972e95b00..1df7f78a3b 100644 --- a/superset-frontend/src/components/DatabaseSelector/index.tsx +++ b/superset-frontend/src/components/DatabaseSelector/index.tsx @@ -302,7 +302,6 @@ export default function DatabaseSelector({ disabled={!currentDb || readOnly} header={{t('Schema')}} labelInValue - lazyLoading={false} loading={loadingSchemas} name="select-schema" placeholder={t('Select schema or type schema name')} diff --git a/superset-frontend/src/components/ListView/Filters/Select.tsx b/superset-frontend/src/components/ListView/Filters/Select.tsx index 525061fd27..ecda25a81f 100644 --- a/superset-frontend/src/components/ListView/Filters/Select.tsx +++ b/superset-frontend/src/components/ListView/Filters/Select.tsx @@ -26,6 +26,7 @@ import { t } from '@superset-ui/core'; import { Select } from 'src/components'; import { Filter, SelectOption } from 'src/components/ListView/types'; import { FormLabel } from 'src/components/Form'; +import AsyncSelect from 'src/components/Select/AsyncSelect'; import { FilterContainer, BaseFilter, FilterHandler } from './Base'; interface SelectFilterProps extends BaseFilter { @@ -86,19 +87,34 @@ function SelectFilter( return ( - {Header}} + labelInValue + onChange={onChange} + onClear={onClear} + options={selects} + placeholder={t('Select or type a value')} + showSearch + value={selectedOption} + /> + )} ); } diff --git a/superset-frontend/src/components/Select/AsyncSelect.test.tsx b/superset-frontend/src/components/Select/AsyncSelect.test.tsx index dc6eff35d9..8a50002c86 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.test.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.test.tsx @@ -60,10 +60,16 @@ const loadOptions = async (search: string, page: number, pageSize: number) => { const start = page * pageSize; const deleteCount = start + pageSize < totalCount ? pageSize : totalCount - start; - const data = OPTIONS.filter(option => option.label.match(search)).splice( - start, - deleteCount, - ); + const searchValue = search.trim().toLowerCase(); + const optionFilterProps = ['label', 'value', 'gender']; + const data = OPTIONS.filter(option => + optionFilterProps.some(prop => { + const optionProp = option?.[prop] + ? String(option[prop]).trim().toLowerCase() + : ''; + return optionProp.includes(searchValue); + }), + ).splice(start, deleteCount); return { data, totalCount: OPTIONS.length, @@ -74,7 +80,7 @@ const defaultProps = { allowClear: true, ariaLabel: ARIA_LABEL, labelInValue: true, - options: OPTIONS, + options: loadOptions, pageSize: 10, showSearch: true, }; @@ -129,17 +135,31 @@ test('displays a header', async () => { expect(screen.getByText(headerText)).toBeInTheDocument(); }); -test('adds a new option if the value is not in the options', async () => { - const { rerender } = render( - , +test('adds a new option if the value is not in the options, when options are empty', async () => { + const loadOptions = jest.fn(async () => ({ data: [], totalCount: 0 })); + render( + , ); await open(); expect(await findSelectOption(OPTIONS[0].label)).toBeInTheDocument(); + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + options.forEach((option, i) => + expect(option).toHaveTextContent(OPTIONS[i].label), + ); +}); - rerender( - , +test('adds a new option if the value is not in the options, when options have values', async () => { + const loadOptions = jest.fn(async () => ({ + data: [OPTIONS[1]], + totalCount: 1, + })); + render( + , ); await open(); + expect(await findSelectOption(OPTIONS[0].label)).toBeInTheDocument(); + expect(await findSelectOption(OPTIONS[1].label)).toBeInTheDocument(); const options = await findAllSelectOptions(); expect(options).toHaveLength(2); options.forEach((option, i) => @@ -147,6 +167,20 @@ test('adds a new option if the value is not in the options', async () => { ); }); +test('does not add a new option if the value is already in the options', async () => { + const loadOptions = jest.fn(async () => ({ + data: [OPTIONS[0]], + totalCount: 1, + })); + render( + , + ); + await open(); + expect(await findSelectOption(OPTIONS[0].label)).toBeInTheDocument(); + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); +}); + test('inverts the selection', async () => { render(); await open(); @@ -155,8 +189,11 @@ test('inverts the selection', async () => { }); test('sort the options by label if no sort comparator is provided', async () => { - const unsortedOptions = [...OPTIONS].sort(() => Math.random()); - render(); + const loadUnsortedOptions = jest.fn(async () => ({ + data: [...OPTIONS].sort(() => Math.random()), + totalCount: 2, + })); + render(); await open(); const options = await findAllSelectOptions(); options.forEach((option, key) => @@ -250,20 +287,23 @@ test('searches for label or value', async () => { render(); const search = option.value; await type(search.toString()); + expect(await findSelectOption(option.label)).toBeInTheDocument(); const options = await findAllSelectOptions(); expect(options.length).toBe(1); expect(options[0]).toHaveTextContent(option.label); }); test('search order exact and startWith match first', async () => { - render(); + render(); + await open(); await type('Her'); + expect(await findSelectOption('Guilherme')).toBeInTheDocument(); const options = await findAllSelectOptions(); expect(options.length).toBe(4); - expect(options[0]?.textContent).toEqual('Her'); - expect(options[1]?.textContent).toEqual('Herme'); - expect(options[2]?.textContent).toEqual('Cher'); - expect(options[3]?.textContent).toEqual('Guilherme'); + expect(options[0]).toHaveTextContent('Her'); + expect(options[1]).toHaveTextContent('Herme'); + expect(options[2]).toHaveTextContent('Cher'); + expect(options[3]).toHaveTextContent('Guilherme'); }); test('ignores case when searching', async () => { @@ -273,17 +313,16 @@ test('ignores case when searching', async () => { }); test('same case should be ranked to the top', async () => { - render( - , - ); + const loadOptions = jest.fn(async () => ({ + data: [ + { value: 'Cac' }, + { value: 'abac' }, + { value: 'acbc' }, + { value: 'CAc' }, + ], + totalCount: 4, + })); + render(); await type('Ac'); const options = await findAllSelectOptions(); expect(options.length).toBe(4); @@ -294,7 +333,7 @@ test('same case should be ranked to the top', async () => { }); test('ignores special keys when searching', async () => { - render(); + render(); await type('{shift}'); expect(screen.queryByText(LOADING)).not.toBeInTheDocument(); }); @@ -303,11 +342,16 @@ test('searches for custom fields', async () => { render( , ); + await open(); await type('Liam'); + // Liam is on the second page. need to wait to fetch options + expect(await findSelectOption('Liam')).toBeInTheDocument(); let options = await findAllSelectOptions(); expect(options.length).toBe(1); expect(options[0]).toHaveTextContent('Liam'); await type('Female'); + // Olivia is on the second page. need to wait to fetch options + expect(await findSelectOption('Olivia')).toBeInTheDocument(); options = await findAllSelectOptions(); expect(options.length).toBe(6); expect(options[0]).toHaveTextContent('Ava'); @@ -317,7 +361,7 @@ test('searches for custom fields', async () => { expect(options[4]).toHaveTextContent('Nikole'); expect(options[5]).toHaveTextContent('Olivia'); await type('1'); - expect(screen.getByText(NO_DATA)).toBeInTheDocument(); + expect(await screen.findByText(NO_DATA)).toBeInTheDocument(); }); test('removes duplicated values', async () => { @@ -332,12 +376,15 @@ test('removes duplicated values', async () => { }); test('renders a custom label', async () => { - const options = [ - { label: 'John', value: 1, customLabel:

John

}, - { label: 'Liam', value: 2, customLabel:

Liam

}, - { label: 'Olivia', value: 3, customLabel:

Olivia

}, - ]; - render(); + const loadOptions = jest.fn(async () => ({ + data: [ + { label: 'John', value: 1, customLabel:

John

}, + { label: 'Liam', value: 2, customLabel:

Liam

}, + { label: 'Olivia', value: 3, customLabel:

Olivia

}, + ], + totalCount: 3, + })); + render(); await open(); expect(screen.getByRole('heading', { name: 'John' })).toBeInTheDocument(); expect(screen.getByRole('heading', { name: 'Liam' })).toBeInTheDocument(); @@ -345,12 +392,15 @@ test('renders a custom label', async () => { }); test('searches for a word with a custom label', async () => { - const options = [ - { label: 'John', value: 1, customLabel:

John

}, - { label: 'Liam', value: 2, customLabel:

Liam

}, - { label: 'Olivia', value: 3, customLabel:

Olivia

}, - ]; - render(); + const loadOptions = jest.fn(async () => ({ + data: [ + { label: 'John', value: 1, customLabel:

John

}, + { label: 'Liam', value: 2, customLabel:

Liam

}, + { label: 'Olivia', value: 3, customLabel:

Olivia

}, + ], + totalCount: 3, + })); + render(); await type('Liam'); const selectOptions = await findAllSelectOptions(); expect(selectOptions.length).toBe(1); @@ -391,7 +441,11 @@ test('does not add a new option if allowNewOptions is false', async () => { }); test('adds the null option when selected in single mode', async () => { - render(); + const loadOptions = jest.fn(async () => ({ + data: [OPTIONS[0], NULL_OPTION], + totalCount: 2, + })); + render(); await open(); userEvent.click(await findSelectOption(NULL_OPTION.label)); const values = await findAllSelectValues(); @@ -399,12 +453,12 @@ test('adds the null option when selected in single mode', async () => { }); test('adds the null option when selected in multiple mode', async () => { + const loadOptions = jest.fn(async () => ({ + data: [OPTIONS[0], NULL_OPTION], + totalCount: 2, + })); render( - , + , ); await open(); userEvent.click(await findSelectOption(OPTIONS[0].label)); diff --git a/superset-frontend/src/components/Select/AsyncSelect.tsx b/superset-frontend/src/components/Select/AsyncSelect.tsx index 98f146f15f..b95f2d8f0d 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.tsx @@ -55,7 +55,6 @@ type PickedSelectProps = Pick< | 'autoFocus' | 'disabled' | 'filterOption' - | 'labelInValue' | 'loading' | 'notFoundContent' | 'onChange' @@ -129,11 +128,10 @@ export interface AsyncSelectProps extends PickedSelectProps { optionFilterProps?: string[]; /** * It defines the options of the Select. - * The options can be static, an array of options. - * The options can also be async, a promise that returns + * The options are async, a promise that returns * an array of options. */ - options: OptionsType | OptionsPagePromise; + options: OptionsPagePromise; /** * It defines how many results should be included * in the query response. @@ -299,7 +297,6 @@ const AsyncSelect = ( filterOption = true, header = null, invertSelection = false, - labelInValue = false, lazyLoading = true, loading, mode = 'single', @@ -322,9 +319,7 @@ const AsyncSelect = ( }: AsyncSelectProps, ref: RefObject, ) => { - const isAsync = typeof options === 'function'; const isSingleMode = mode === 'single'; - const shouldShowSearch = isAsync || allowNewOptions ? true : showSearch; const [selectValue, setSelectValue] = useState(value); const [inputValue, setInputValue] = useState(''); const [isLoading, setIsLoading] = useState(loading); @@ -360,8 +355,8 @@ const AsyncSelect = ( sortSelectedFirst(a, b) || // Only apply the custom sorter in async mode because we should // preserve the options order as much as possible. - (isAsync ? sortComparator(a, b, '') : 0), - [isAsync, sortComparator, sortSelectedFirst], + sortComparator(a, b, ''), + [sortComparator, sortSelectedFirst], ); const initialOptions = useMemo( @@ -528,7 +523,6 @@ const AsyncSelect = ( setSelectOptions(newOptions); } if ( - isAsync && !allValuesLoaded && loadingEnabled && !fetchedQueries.current.has(getQueryCacheKey(searchValue, 0, pageSize)) @@ -546,7 +540,7 @@ const AsyncSelect = ( vScroll.scrollTop > (vScroll.scrollHeight - vScroll.offsetHeight) * 0.7; const hasMoreData = page * pageSize + pageSize < totalCount; - if (!isLoading && isAsync && hasMoreData && thresholdReached) { + if (!isLoading && hasMoreData && thresholdReached) { const newPage = page + 1; fetchPage(inputValue, newPage); } @@ -575,30 +569,26 @@ const AsyncSelect = ( const handleOnDropdownVisibleChange = (isDropdownVisible: boolean) => { setIsDropdownVisible(isDropdownVisible); - if (isAsync) { - // loading is enabled when dropdown is open, - // disabled when dropdown is closed - if (loadingEnabled !== isDropdownVisible) { - setLoadingEnabled(isDropdownVisible); - } - // when closing dropdown, always reset loading state - if (!isDropdownVisible && isLoading) { - // delay is for the animation of closing the dropdown - // so the dropdown doesn't flash between "Loading..." and "No data" - // before closing. - setTimeout(() => { - setIsLoading(false); - }, 250); - } + // loading is enabled when dropdown is open, + // disabled when dropdown is closed + if (loadingEnabled !== isDropdownVisible) { + setLoadingEnabled(isDropdownVisible); + } + // when closing dropdown, always reset loading state + if (!isDropdownVisible && isLoading) { + // delay is for the animation of closing the dropdown + // so the dropdown doesn't flash between "Loading..." and "No data" + // before closing. + setTimeout(() => { + setIsLoading(false); + }, 250); } // if no search input value, force sort options because it won't be sorted by // `filterSort`. if (isDropdownVisible && !inputValue && selectOptions.length > 1) { - const sortedOptions = isAsync - ? selectOptions.slice().sort(sortComparatorForNoSearch) - : // if not in async mode, revert to the original select options - // (with selected options still sorted to the top) - initialOptionsSorted; + const sortedOptions = selectOptions + .slice() + .sort(sortComparatorForNoSearch); if (!isEqual(sortedOptions, selectOptions)) { setSelectOptions(sortedOptions); } @@ -627,7 +617,7 @@ const AsyncSelect = ( if (isLoading) { return ; } - if (shouldShowSearch && isDropdownVisible) { + if (showSearch && isDropdownVisible) { return ; } return ; @@ -660,7 +650,7 @@ const AsyncSelect = ( ); useEffect(() => { - if (isAsync && loadingEnabled && allowFetch) { + if (loadingEnabled && allowFetch) { // trigger fetch every time inputValue changes if (inputValue) { debouncedFetchPage(inputValue, 0); @@ -668,14 +658,7 @@ const AsyncSelect = ( fetchPage('', 0); } } - }, [ - isAsync, - loadingEnabled, - fetchPage, - allowFetch, - inputValue, - debouncedFetchPage, - ]); + }, [loadingEnabled, fetchPage, allowFetch, inputValue, debouncedFetchPage]); useEffect(() => { if (loading !== undefined && loading !== isLoading) { @@ -706,20 +689,20 @@ const AsyncSelect = ( getPopupContainer={ getPopupContainer || (triggerNode => triggerNode.parentNode) } - labelInValue={isAsync || labelInValue} + labelInValue maxTagCount={MAX_TAG_COUNT} mode={mappedMode} notFoundContent={isLoading ? t('Loading...') : notFoundContent} onDeselect={handleOnDeselect} onDropdownVisibleChange={handleOnDropdownVisibleChange} - onPopupScroll={isAsync ? handlePagination : undefined} - onSearch={shouldShowSearch ? handleOnSearch : undefined} + onPopupScroll={handlePagination} + onSearch={showSearch ? handleOnSearch : undefined} onSelect={handleOnSelect} onClear={handleClear} onChange={onChange} options={hasCustomLabels ? undefined : fullSelectOptions} placeholder={placeholder} - showSearch={shouldShowSearch} + showSearch={showSearch} showArrow tokenSeparators={tokenSeparators || TOKEN_SEPARATORS} value={selectValue} diff --git a/superset-frontend/src/components/Select/Select.stories.tsx b/superset-frontend/src/components/Select/Select.stories.tsx index efcd91c0c3..b75e1ff28b 100644 --- a/superset-frontend/src/components/Select/Select.stories.tsx +++ b/superset-frontend/src/components/Select/Select.stories.tsx @@ -16,11 +16,22 @@ * specific language governing permissions and limitations * under the License. */ -import React, { ReactNode, useState, useCallback, useRef } from 'react'; +import React, { + ReactNode, + useState, + useCallback, + useRef, + useMemo, +} from 'react'; import Button from 'src/components/Button'; import ControlHeader from 'src/explore/components/ControlHeader'; -import AsyncSelect, { AsyncSelectProps, AsyncSelectRef } from './AsyncSelect'; -import Select, { SelectProps, OptionsTypePage, OptionsType } from './Select'; +import AsyncSelect, { + AsyncSelectProps, + AsyncSelectRef, + OptionsTypePage, +} from './AsyncSelect'; + +import Select, { SelectProps, OptionsType } from './Select'; export default { title: 'Select', @@ -452,6 +463,11 @@ export const AsynchronousSelect = ({ reject(new Error('Error while fetching the names from the server')); }); + const initialValue = useMemo( + () => ({ label: 'Valentina', value: 'Valentina' }), + [], + ); + return ( <>
{ expect(screen.getByText(headerText)).toBeInTheDocument(); }); -test('adds a new option if the value is not in the options', async () => { - const { rerender } = render( - ); await open(); expect(await findSelectOption(OPTIONS[0].label)).toBeInTheDocument(); + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + options.forEach((option, i) => + expect(option).toHaveTextContent(OPTIONS[i].label), + ); +}); - rerender( +test('adds a new option if the value is not in the options, when options have values', async () => { + render( , + ); + await open(); + expect(await findSelectOption(OPTIONS[0].label)).toBeInTheDocument(); + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); +}); + test('inverts the selection', async () => { render(