-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(select): keep options order when not in async mode #19085
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
import isEqualArray from './isEqualArray'; | ||
|
||
test('isEqualArray', () => { | ||
expect(isEqualArray([], [])).toBe(true); | ||
expect(isEqualArray([1, 2], [1, 2])).toBe(true); | ||
const item1 = { a: 1 }; | ||
expect(isEqualArray([item1], [item1])).toBe(true); | ||
expect(isEqualArray(null, undefined)).toBe(true); | ||
// compare is shallow | ||
expect(isEqualArray([{ a: 1 }], [{ a: 1 }])).toBe(false); | ||
expect(isEqualArray(null, [])).toBe(false); | ||
expect(isEqualArray([1, 2], [])).toBe(false); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,9 +23,11 @@ export default function isEqualArray<T extends unknown[] | undefined | null>( | |
return ( | ||
arrA === arrB || | ||
(!arrA && !arrB) || | ||
(arrA && | ||
!!( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making sure return result is always boolean |
||
arrA && | ||
arrB && | ||
arrA.length === arrB.length && | ||
arrA.every((x, i) => x === arrB[i])) | ||
arrA.every((x, i) => x === arrB[i]) | ||
) | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,6 +99,18 @@ const findAllSelectValues = () => | |
|
||
const clearAll = () => userEvent.click(screen.getByLabelText('close-circle')); | ||
|
||
const matchOrder = async (expectedLabels: string[]) => { | ||
const actualLabels: string[] = []; | ||
(await findAllSelectOptions()).forEach(option => { | ||
actualLabels.push(option.textContent || ''); | ||
}); | ||
// menu is a virtual list, which means it may not render all options | ||
expect(actualLabels.slice(0, expectedLabels.length)).toEqual( | ||
expectedLabels.slice(0, actualLabels.length), | ||
); | ||
return true; | ||
}; | ||
|
||
const type = (text: string) => { | ||
const select = getSelect(); | ||
userEvent.clear(select); | ||
|
@@ -169,34 +181,64 @@ test('sort the options using a custom sort comparator', async () => { | |
}); | ||
}); | ||
|
||
test('displays the selected values first', async () => { | ||
render(<Select {...defaultProps} mode="multiple" />); | ||
const option3 = OPTIONS[2].label; | ||
const option8 = OPTIONS[7].label; | ||
test('should sort selected to top when in single mode', async () => { | ||
render(<Select {...defaultProps} mode="single" />); | ||
const originalLabels = OPTIONS.map(option => option.label); | ||
await open(); | ||
userEvent.click(await findSelectOption(originalLabels[1])); | ||
// after selection, keep the original order | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice improvement to the test! |
||
expect(await matchOrder(originalLabels)).toBe(true); | ||
|
||
// order selected to top when reopen | ||
await type('{esc}'); | ||
await open(); | ||
let labels = originalLabels.slice(); | ||
labels = labels.splice(1, 1).concat(labels); | ||
expect(await matchOrder(labels)).toBe(true); | ||
|
||
// keep clicking other items, the updated order should still based on | ||
// original order | ||
userEvent.click(await findSelectOption(originalLabels[5])); | ||
await matchOrder(labels); | ||
await type('{esc}'); | ||
await open(); | ||
userEvent.click(await findSelectOption(option3)); | ||
userEvent.click(await findSelectOption(option8)); | ||
labels = originalLabels.slice(); | ||
labels = labels.splice(5, 1).concat(labels); | ||
expect(await matchOrder(labels)).toBe(true); | ||
|
||
// should revert to original order | ||
clearAll(); | ||
await type('{esc}'); | ||
await open(); | ||
const sortedOptions = await findAllSelectOptions(); | ||
expect(sortedOptions[0]).toHaveTextContent(option3); | ||
expect(sortedOptions[1]).toHaveTextContent(option8); | ||
expect(await matchOrder(originalLabels)).toBe(true); | ||
}); | ||
|
||
test('displays the original order when unselecting', async () => { | ||
test('should sort selected to the top when in multi mode', async () => { | ||
render(<Select {...defaultProps} mode="multiple" />); | ||
const option3 = OPTIONS[2].label; | ||
const option8 = OPTIONS[7].label; | ||
const originalLabels = OPTIONS.map(option => option.label); | ||
let labels = originalLabels.slice(); | ||
|
||
await open(); | ||
userEvent.click(await findSelectOption(option3)); | ||
userEvent.click(await findSelectOption(option8)); | ||
userEvent.click(await findSelectOption(labels[1])); | ||
expect(await matchOrder(labels)).toBe(true); | ||
|
||
await type('{esc}'); | ||
await open(); | ||
labels = labels.splice(1, 1).concat(labels); | ||
expect(await matchOrder(labels)).toBe(true); | ||
|
||
await open(); | ||
userEvent.click(await findSelectOption(labels[5])); | ||
await type('{esc}'); | ||
await open(); | ||
labels = [labels.splice(0, 1)[0], labels.splice(4, 1)[0]].concat(labels); | ||
expect(await matchOrder(labels)).toBe(true); | ||
|
||
// should revert to original order | ||
clearAll(); | ||
await type('{esc}'); | ||
await open(); | ||
const options = await findAllSelectOptions(); | ||
options.forEach((option, key) => | ||
expect(option).toHaveTextContent(OPTIONS[key].label), | ||
); | ||
expect(await matchOrder(originalLabels)).toBe(true); | ||
}); | ||
|
||
test('searches for label or value', async () => { | ||
|
@@ -540,17 +582,35 @@ test('async - changes the selected item in single mode', async () => { | |
test('async - deselects an item in multiple mode', async () => { | ||
render(<Select {...defaultProps} options={loadOptions} mode="multiple" />); | ||
await open(); | ||
const [firstOption, secondOption] = OPTIONS; | ||
userEvent.click(await findSelectOption(firstOption.label)); | ||
userEvent.click(await findSelectOption(secondOption.label)); | ||
const option3 = OPTIONS[2]; | ||
const option8 = OPTIONS[7]; | ||
userEvent.click(await findSelectOption(option8.label)); | ||
userEvent.click(await findSelectOption(option3.label)); | ||
|
||
let options = await findAllSelectOptions(); | ||
expect(options).toHaveLength(Math.min(defaultProps.pageSize, OPTIONS.length)); | ||
expect(options[0]).toHaveTextContent(OPTIONS[0].label); | ||
expect(options[1]).toHaveTextContent(OPTIONS[1].label); | ||
|
||
await type('{esc}'); | ||
await open(); | ||
|
||
// should rank selected options to the top after menu closes | ||
options = await findAllSelectOptions(); | ||
expect(options).toHaveLength(Math.min(defaultProps.pageSize, OPTIONS.length)); | ||
expect(options[0]).toHaveTextContent(option3.label); | ||
expect(options[1]).toHaveTextContent(option8.label); | ||
|
||
let values = await findAllSelectValues(); | ||
expect(values.length).toBe(2); | ||
expect(values[0]).toHaveTextContent(firstOption.label); | ||
expect(values[1]).toHaveTextContent(secondOption.label); | ||
userEvent.click(await findSelectOption(firstOption.label)); | ||
expect(values).toHaveLength(2); | ||
// should keep the order by which the options were selected | ||
expect(values[0]).toHaveTextContent(option8.label); | ||
expect(values[1]).toHaveTextContent(option3.label); | ||
|
||
userEvent.click(await findSelectOption(option3.label)); | ||
values = await findAllSelectValues(); | ||
expect(values.length).toBe(1); | ||
expect(values[0]).toHaveTextContent(secondOption.label); | ||
expect(values[0]).toHaveTextContent(option8.label); | ||
}); | ||
|
||
test('async - adds a new option if none is available and allowNewOptions is true', async () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,6 +156,10 @@ export interface SelectProps extends PickedSelectProps { | |
* Works in async mode only (See the options property). | ||
*/ | ||
onError?: (error: string) => void; | ||
/** | ||
* Customize how filtered options are sorted while users search. | ||
* Will not apply to predefined `options` array when users are not searching. | ||
*/ | ||
sortComparator?: typeof DEFAULT_SORT_COMPARATOR; | ||
} | ||
|
||
|
@@ -314,8 +318,6 @@ const Select = ( | |
const isAsync = typeof options === 'function'; | ||
const isSingleMode = mode === 'single'; | ||
const shouldShowSearch = isAsync || allowNewOptions ? true : showSearch; | ||
const initialOptions = | ||
options && Array.isArray(options) ? options.slice() : EMPTY_OPTIONS; | ||
const [selectValue, setSelectValue] = useState(value); | ||
const [inputValue, setInputValue] = useState(''); | ||
const [isLoading, setIsLoading] = useState(loading); | ||
|
@@ -346,13 +348,27 @@ const Select = ( | |
sortSelectedFirst(a, b) || sortComparator(a, b, inputValue), | ||
[inputValue, sortComparator, sortSelectedFirst], | ||
); | ||
const sortComparatorWithoutSearch = useCallback( | ||
const sortComparatorForNoSearch = useCallback( | ||
(a: AntdLabeledValue, b: AntdLabeledValue) => | ||
sortSelectedFirst(a, b) || sortComparator(a, b, ''), | ||
[sortComparator, sortSelectedFirst], | ||
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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add the following docs (or something similar) to
|
||
[isAsync, sortComparator, sortSelectedFirst], | ||
); | ||
|
||
const initialOptions = useMemo( | ||
() => (options && Array.isArray(options) ? options.slice() : EMPTY_OPTIONS), | ||
[options], | ||
); | ||
const initialOptionsSorted = useMemo( | ||
() => initialOptions.slice().sort(sortComparatorForNoSearch), | ||
[initialOptions, sortComparatorForNoSearch], | ||
); | ||
|
||
const [selectOptions, setSelectOptions] = | ||
useState<OptionsType>(initialOptions); | ||
useState<OptionsType>(initialOptionsSorted); | ||
|
||
// add selected values to options list if they are not in it | ||
const fullSelectOptions = useMemo(() => { | ||
const missingValues: OptionsType = ensureIsArray(selectValue) | ||
|
@@ -433,13 +449,13 @@ const Select = ( | |
mergedData = prevOptions | ||
.filter(previousOption => !dataValues.has(previousOption.value)) | ||
.concat(data) | ||
.sort(sortComparatorWithoutSearch); | ||
.sort(sortComparatorForNoSearch); | ||
return mergedData; | ||
}); | ||
} | ||
return mergedData; | ||
}, | ||
[sortComparatorWithoutSearch], | ||
[sortComparatorForNoSearch], | ||
); | ||
|
||
const fetchPage = useMemo( | ||
|
@@ -575,11 +591,13 @@ const Select = ( | |
} | ||
// if no search input value, force sort options because it won't be sorted by | ||
// `filterSort`. | ||
if (isDropdownVisible && !inputValue && fullSelectOptions.length > 0) { | ||
const sortedOptions = [...fullSelectOptions].sort( | ||
sortComparatorWithSearch, | ||
); | ||
if (!isEqual(sortedOptions, fullSelectOptions)) { | ||
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; | ||
if (!isEqual(sortedOptions, selectOptions)) { | ||
setSelectOptions(sortedOptions); | ||
} | ||
} | ||
|
@@ -624,10 +642,8 @@ const Select = ( | |
// when `options` list is updated from component prop, reset states | ||
fetchedQueries.current.clear(); | ||
setAllValuesLoaded(false); | ||
setSelectOptions( | ||
options && Array.isArray(options) ? options : EMPTY_OPTIONS, | ||
); | ||
}, [options]); | ||
setSelectOptions(initialOptions); | ||
}, [initialOptions]); | ||
|
||
useEffect(() => { | ||
setSelectValue(value); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically a faster version of
lodash.isEqual
where we only compare items in arrays shallowly. Also used in Table chart.