Skip to content

Commit

Permalink
fix: Duplicated options in Select when using numerical values (#24906)
Browse files Browse the repository at this point in the history
  • Loading branch information
michael-s-molina authored Aug 11, 2023
1 parent a1e32db commit b621ee9
Show file tree
Hide file tree
Showing 8 changed files with 260 additions and 151 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ export function applyNativeFilterValueWithIndex(index: number, value: string) {
cy.get(nativeFilters.filterFromDashboardView.filterValueInput)
.eq(index)
.should('exist', { timeout: 10000 })
.type(`${value}{enter}`);
.type(`${value}{enter}`, { force: true });
// click the title to dismiss shown options
cy.get(nativeFilters.filterFromDashboardView.filterName)
.eq(index)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe('Advanced analytics', () => {

cy.get('[data-test=time_compare]')
.find('input[type=search]')
.clear()
.type('1 year{enter}');

cy.get('button[data-test="run-query-button"]').click();
Expand Down
34 changes: 0 additions & 34 deletions superset-frontend/src/components/Select/AsyncSelect.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import React, {
import Button from 'src/components/Button';
import AsyncSelect from './AsyncSelect';
import {
SelectOptionsType,
AsyncSelectProps,
AsyncSelectRef,
SelectOptionsTypePage,
Expand All @@ -39,40 +38,7 @@ export default {

const DEFAULT_WIDTH = 200;

const options: SelectOptionsType = [
{
label: 'Such an incredibly awesome long long label',
value: 'Such an incredibly awesome long long label',
custom: 'Secret custom prop',
},
{
label: 'Another incredibly awesome long long label',
value: 'Another incredibly awesome long long label',
},
{
label: 'JSX Label',
customLabel: <div style={{ color: 'red' }}>JSX Label</div>,
value: 'JSX Label',
},
{ label: 'A', value: 'A' },
{ label: 'B', value: 'B' },
{ label: 'C', value: 'C' },
{ label: 'D', value: 'D' },
{ label: 'E', value: 'E' },
{ label: 'F', value: 'F' },
{ label: 'G', value: 'G' },
{ label: 'H', value: 'H' },
{ label: 'I', value: 'I' },
];

const ARG_TYPES = {
options: {
defaultValue: options,
description: `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 an array of options.
`,
},
ariaLabel: {
description: `It adds the aria-label tag for accessibility standards.
Must be plain English and localized.
Expand Down
63 changes: 49 additions & 14 deletions superset-frontend/src/components/Select/AsyncSelect.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,14 @@
* under the License.
*/
import React from 'react';
import { render, screen, waitFor, within } from 'spec/helpers/testing-library';
import {
createEvent,
fireEvent,
render,
screen,
waitFor,
within,
} from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import { AsyncSelect } from 'src/components';

Expand Down Expand Up @@ -93,6 +100,9 @@ const getElementsByClassName = (className: string) =>

const getSelect = () => screen.getByRole('combobox', { name: ARIA_LABEL });

const getAllSelectOptions = () =>
getElementsByClassName('.ant-select-item-option-content');

const findSelectOption = (text: string) =>
waitFor(() =>
within(getElementByClassName('.rc-virtual-list')).getByText(text),
Expand Down Expand Up @@ -323,12 +333,14 @@ test('same case should be ranked to the top', async () => {
}));
render(<AsyncSelect {...defaultProps} options={loadOptions} />);
await type('Ac');
const options = await findAllSelectOptions();
expect(options.length).toBe(4);
expect(options[0]?.textContent).toEqual('acbc');
expect(options[1]?.textContent).toEqual('CAc');
expect(options[2]?.textContent).toEqual('abac');
expect(options[3]?.textContent).toEqual('Cac');
await waitFor(() => {
const options = getAllSelectOptions();
expect(options.length).toBe(4);
expect(options[0]?.textContent).toEqual('acbc');
expect(options[1]?.textContent).toEqual('CAc');
expect(options[2]?.textContent).toEqual('abac');
expect(options[3]?.textContent).toEqual('Cac');
});
});

test('ignores special keys when searching', async () => {
Expand Down Expand Up @@ -365,7 +377,13 @@ test('searches for custom fields', async () => {

test('removes duplicated values', async () => {
render(<AsyncSelect {...defaultProps} mode="multiple" allowNewOptions />);
await type('a,b,b,b,c,d,d');
const input = getElementByClassName('.ant-select-selection-search-input');
const paste = createEvent.paste(input, {
clipboardData: {
getData: () => 'a,b,b,b,c,d,d',
},
});
fireEvent(input, paste);
const values = await findAllSelectValues();
expect(values.length).toBe(4);
expect(values[0]).toHaveTextContent('a');
Expand Down Expand Up @@ -601,7 +619,9 @@ test('does not show "No data" when allowNewOptions is true and a new option is e
render(<AsyncSelect {...defaultProps} allowNewOptions />);
await open();
await type(NEW_OPTION);
expect(screen.queryByText(NO_DATA)).not.toBeInTheDocument();
await waitFor(() =>
expect(screen.queryByText(NO_DATA)).not.toBeInTheDocument(),
);
});

test('sets a initial value in single mode', async () => {
Expand Down Expand Up @@ -690,12 +710,9 @@ test('does not fire a new request for the same search input', async () => {
<AsyncSelect {...defaultProps} options={loadOptions} fetchOnlyOnSearch />,
);
await type('search');
expect(await screen.findByText(NO_DATA)).toBeInTheDocument();
expect(loadOptions).toHaveBeenCalledTimes(1);
clearAll();
await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1));
await type('search');
expect(await screen.findByText(LOADING)).toBeInTheDocument();
expect(loadOptions).toHaveBeenCalledTimes(1);
await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1));
});

test('does not fire a new request if all values have been fetched', async () => {
Expand Down Expand Up @@ -823,6 +840,24 @@ test('does not fire onChange when searching but no selection', async () => {
expect(onChange).toHaveBeenCalledTimes(1);
});

test('does not duplicate options when using numeric values', async () => {
render(
<AsyncSelect
{...defaultProps}
mode="multiple"
options={async () => ({
data: [
{ label: '1', value: 1 },
{ label: '2', value: 2 },
],
totalCount: 2,
})}
/>,
);
await type('1');
await waitFor(() => expect(getAllSelectOptions().length).toBe(1));
});

/*
TODO: Add tests that require scroll interaction. Needs further investigation.
- Fetches more data when scrolling and more data is available
Expand Down
75 changes: 47 additions & 28 deletions superset-frontend/src/components/Select/AsyncSelect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,15 @@ import React, {
useRef,
useCallback,
useImperativeHandle,
ClipboardEvent,
} from 'react';
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';
import { isEqual, uniq } from 'lodash';
import Icons from 'src/components/Icons';
import { getClientErrorObject } from 'src/utils/getClientErrorObject';
import { SLOW_DEBOUNCE } from 'src/constants';
import { FAST_DEBOUNCE, SLOW_DEBOUNCE } from 'src/constants';
import {
getValue,
hasOption,
Expand Down Expand Up @@ -122,14 +123,15 @@ const AsyncSelect = forwardRef(
onClear,
onDropdownVisibleChange,
onDeselect,
onSearch,
onSelect,
optionFilterProps = ['label', 'value'],
options,
pageSize = DEFAULT_PAGE_SIZE,
placeholder = t('Select ...'),
showSearch = true,
sortComparator = DEFAULT_SORT_COMPARATOR,
tokenSeparators,
tokenSeparators = TOKEN_SEPARATORS,
value,
getPopupContainer,
oneLine,
Expand All @@ -150,11 +152,7 @@ const AsyncSelect = forwardRef(
const [allValuesLoaded, setAllValuesLoaded] = useState(false);
const selectValueRef = useRef(selectValue);
const fetchedQueries = useRef(new Map<string, number>());
const mappedMode = isSingleMode
? undefined
: allowNewOptions
? 'tags'
: 'multiple';
const mappedMode = isSingleMode ? undefined : 'multiple';
const allowFetch = !fetchOnlyOnSearch || inputValue;
const [maxTagCount, setMaxTagCount] = useState(
propsMaxTagCount ?? MAX_TAG_COUNT,
Expand Down Expand Up @@ -253,6 +251,14 @@ const AsyncSelect = forwardRef(
const array = selectValue as (string | number)[];
setSelectValue(array.filter(element => element !== value));
}
// removes new option
if (option.isNewOption) {
setSelectOptions(
fullSelectOptions.filter(
option => getValue(option.value) !== getValue(value),
),
);
}
}
fireOnChange();
onDeselect?.(value, option);
Expand Down Expand Up @@ -341,9 +347,9 @@ const AsyncSelect = forwardRef(
[fetchPage],
);

const handleOnSearch = (search: string) => {
const handleOnSearch = debounce((search: string) => {
const searchValue = search.trim();
if (allowNewOptions && isSingleMode) {
if (allowNewOptions) {
const newOption = searchValue &&
!hasOption(searchValue, fullSelectOptions, true) && {
label: searchValue,
Expand All @@ -368,7 +374,10 @@ const AsyncSelect = forwardRef(
setIsLoading(!(fetchOnlyOnSearch && !searchValue));
}
setInputValue(search);
};
onSearch?.(searchValue);
}, FAST_DEBOUNCE);

useEffect(() => () => handleOnSearch.cancel(), [handleOnSearch]);

const handlePagination = (e: UIEvent<HTMLElement>) => {
const vScroll = e.currentTarget;
Expand Down Expand Up @@ -439,19 +448,7 @@ const AsyncSelect = forwardRef(
};

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 || []);
}
setInputValue('');
onBlur?.(event);
};

Expand Down Expand Up @@ -526,6 +523,28 @@ const AsyncSelect = forwardRef(
[ref],
);

const onPaste = (e: ClipboardEvent<HTMLInputElement>) => {
const pastedText = e.clipboardData.getData('text');
if (isSingleMode) {
setSelectValue({ label: pastedText, value: pastedText });
} else {
const token = tokenSeparators.find(token => pastedText.includes(token));
const array = token ? uniq(pastedText.split(token)) : [pastedText];
setSelectValue(previous => [
...((previous || []) as AntdLabeledValue[]),
...array.map<AntdLabeledValue>(value => ({
label: value,
value,
})),
]);
}
};

const shouldRenderChildrenOptions = useMemo(
() => hasCustomLabels(fullSelectOptions),
[fullSelectOptions],
);

return (
<StyledContainer headerPosition={headerPosition}>
{header && (
Expand All @@ -549,17 +568,17 @@ const AsyncSelect = forwardRef(
onBlur={handleOnBlur}
onDeselect={handleOnDeselect}
onDropdownVisibleChange={handleOnDropdownVisibleChange}
// @ts-ignore
onPaste={onPaste}
onPopupScroll={handlePagination}
onSearch={showSearch ? handleOnSearch : undefined}
onSelect={handleOnSelect}
onClear={handleClear}
options={
hasCustomLabels(fullSelectOptions) ? undefined : fullSelectOptions
}
options={shouldRenderChildrenOptions ? undefined : fullSelectOptions}
placeholder={placeholder}
showSearch={showSearch}
showArrow
tokenSeparators={tokenSeparators || TOKEN_SEPARATORS}
tokenSeparators={tokenSeparators}
value={selectValue}
suffixIcon={getSuffixIcon(isLoading, showSearch, isDropdownVisible)}
menuItemSelectedIcon={
Expand Down
5 changes: 5 additions & 0 deletions superset-frontend/src/components/Select/Select.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ const ARG_TYPES = {
disable: true,
},
},
mappedMode: {
table: {
disable: true,
},
},
mode: {
description: `It defines whether the Select should allow for
the selection of multiple options or single. Single by default.
Expand Down
Loading

0 comments on commit b621ee9

Please sign in to comment.