Skip to content
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

fix: Order of Select items when unselecting #17169

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions superset-frontend/src/addSlice/AddSliceContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -256,15 +256,11 @@ export default class AddSliceContainer extends React.PureComponent<
customLabel: ReactNode;
label: string;
value: string;
}[] = response.json.result
.map((item: Dataset) => ({
value: `${item.id}__${item.datasource_type}`,
customLabel: this.newLabel(item),
label: item.table_name,
}))
.sort((a: { label: string }, b: { label: string }) =>
a.label.localeCompare(b.label),
);
}[] = response.json.result.map((item: Dataset) => ({
value: `${item.id}__${item.datasource_type}`,
customLabel: this.newLabel(item),
label: item.table_name,
}));
return {
data: list,
totalCount: response.json.count,
Expand Down
16 changes: 6 additions & 10 deletions superset-frontend/src/components/DatabaseSelector/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -201,22 +201,18 @@ export default function DatabaseSelector({
// TODO: Would be nice to add pagination in a follow-up. Needs endpoint changes.
SupersetClient.get({ endpoint })
.then(({ json }) => {
const options = json.result
.map((s: string) => ({
value: s,
label: s,
title: s,
}))
.sort((a: { label: string }, b: { label: string }) =>
a.label.localeCompare(b.label),
);
const options = json.result.map((s: string) => ({
value: s,
label: s,
title: s,
}));
if (onSchemasLoad) {
onSchemasLoad(options);
}
setSchemaOptions(options);
setLoadingSchemas(false);
})
.catch(e => {
.catch(() => {
setLoadingSchemas(false);
handleError(t('There was an error loading the schemas'));
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ const USERS = [
'Claire',
'Benedetta',
'Ilenia',
];
].sort();

export const AsyncSelect = ({
fetchOnlyOnSearch,
Expand Down Expand Up @@ -429,6 +429,7 @@ export const AsyncSelect = ({
};

AsyncSelect.args = {
allowClear: false,
allowNewOptions: false,
fetchOnlyOnSearch: false,
pageSize: 10,
Expand Down
73 changes: 61 additions & 12 deletions superset-frontend/src/components/Select/Select.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const OPTIONS = [
{ label: 'Irfan', value: 18, gender: 'Male' },
{ label: 'George', value: 19, gender: 'Male' },
{ label: 'Ashfaq', value: 20, gender: 'Male' },
];
].sort((option1, option2) => option1.label.localeCompare(option2.label));

const loadOptions = async (search: string, page: number, pageSize: number) => {
const totalCount = OPTIONS.length;
Expand Down Expand Up @@ -100,6 +100,8 @@ const findSelectValue = () =>
const findAllSelectValues = () =>
waitFor(() => getElementsByClassName('.ant-select-selection-item'));

const clearAll = () => userEvent.click(screen.getByLabelText('close-circle'));

const type = (text: string) => {
const select = getSelect();
userEvent.clear(select);
Expand Down Expand Up @@ -127,6 +129,37 @@ test('inverts the selection', async () => {
expect(await screen.findByLabelText('stop')).toBeInTheDocument();
});

test('sort the options by label if no sort comparator is provided', async () => {
const unsortedOptions = [...OPTIONS].sort(() => Math.random());
render(<Select {...defaultProps} options={unsortedOptions} />);
await open();
const options = await findAllSelectOptions();
options.forEach((option, key) =>
expect(option).toHaveTextContent(OPTIONS[key].label),
);
});

test('sort the options using a custom sort comparator', async () => {
const sortComparator = (
option1: typeof OPTIONS[0],
option2: typeof OPTIONS[0],
) => option1.gender.localeCompare(option2.gender);
render(
<Select
{...defaultProps}
options={loadOptions}
sortComparator={sortComparator}
/>,
);
await open();
const options = await findAllSelectOptions();
const optionsPage = OPTIONS.slice(0, defaultProps.pageSize);
const sortedOptions = optionsPage.sort(sortComparator);
options.forEach((option, key) =>
expect(option).toHaveTextContent(sortedOptions[key].label),
);
});

test('displays the selected values first', async () => {
render(<Select {...defaultProps} mode="multiple" />);
const option3 = OPTIONS[2].label;
Expand All @@ -141,6 +174,22 @@ test('displays the selected values first', async () => {
expect(sortedOptions[1]).toHaveTextContent(option8);
});

test('displays the original order when unselecting', async () => {
render(<Select {...defaultProps} mode="multiple" />);
const option3 = OPTIONS[2].label;
const option8 = OPTIONS[7].label;
await open();
userEvent.click(await findSelectOption(option3));
userEvent.click(await findSelectOption(option8));
await type('{esc}');
clearAll();
await open();
const options = await findAllSelectOptions();
options.forEach((option, key) =>
expect(option).toHaveTextContent(OPTIONS[key].label),
);
});

test('searches for label or value', async () => {
const option = OPTIONS[11];
render(<Select {...defaultProps} />);
Expand Down Expand Up @@ -172,11 +221,11 @@ test('searches for custom fields', async () => {
await type('Female');
options = await findAllSelectOptions();
expect(options.length).toBe(5);
expect(options[0]).toHaveTextContent('Olivia');
expect(options[1]).toHaveTextContent('Emma');
expect(options[2]).toHaveTextContent('Ava');
expect(options[3]).toHaveTextContent('Charlotte');
expect(options[4]).toHaveTextContent('Nikole');
expect(options[0]).toHaveTextContent('Ava');
expect(options[1]).toHaveTextContent('Charlotte');
expect(options[2]).toHaveTextContent('Emma');
expect(options[3]).toHaveTextContent('Nikole');
expect(options[4]).toHaveTextContent('Olivia');
await type('1');
expect(screen.getByText(NO_DATA)).toBeInTheDocument();
});
Expand Down Expand Up @@ -227,7 +276,7 @@ test('clear all the values', async () => {
onClear={onClear}
/>,
);
userEvent.click(screen.getByLabelText('close-circle'));
clearAll();
expect(onClear).toHaveBeenCalled();
const values = await findAllSelectValues();
expect(values.length).toBe(0);
Expand Down Expand Up @@ -378,8 +427,8 @@ test('static - searches for an item', async () => {
await type(search);
const options = await findAllSelectOptions();
expect(options.length).toBe(2);
expect(options[0]).toHaveTextContent('Olivia');
expect(options[1]).toHaveTextContent('Oliver');
expect(options[0]).toHaveTextContent('Oliver');
expect(options[1]).toHaveTextContent('Olivia');
});

test('async - renders the select with default props', () => {
Expand Down Expand Up @@ -546,8 +595,8 @@ test('async - searches for an item already loaded', async () => {
await waitForElementToBeRemoved(screen.getByText(LOADING));
const options = await findAllSelectOptions();
expect(options.length).toBe(2);
expect(options[0]).toHaveTextContent('Olivia');
expect(options[1]).toHaveTextContent('Oliver');
expect(options[0]).toHaveTextContent('Oliver');
expect(options[1]).toHaveTextContent('Olivia');
});

test('async - searches for an item in a page not loaded', async () => {
Expand Down Expand Up @@ -599,7 +648,7 @@ test('async - does not fire a new request for the same search input', async () =
await type('search');
expect(await screen.findByText(NO_DATA)).toBeInTheDocument();
expect(loadOptions).toHaveBeenCalledTimes(1);
userEvent.click(screen.getByLabelText('close-circle'));
clearAll();
await type('search');
expect(await screen.findByText(NO_DATA)).toBeInTheDocument();
expect(loadOptions).toHaveBeenCalledTimes(1);
Expand Down
95 changes: 67 additions & 28 deletions superset-frontend/src/components/Select/Select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ export interface SelectProps extends PickedSelectProps {
invertSelection?: boolean;
fetchOnlyOnSearch?: boolean;
onError?: (error: string) => void;
sortComparator?: (a: AntdLabeledValue, b: AntdLabeledValue) => number;
}

const StyledContainer = styled.div`
Expand Down Expand Up @@ -168,6 +169,30 @@ const Error = ({ error }: { error: string }) => (
</StyledError>
);

const defaultSortComparator = (a: AntdLabeledValue, b: AntdLabeledValue) => {
if (typeof a.label === 'string' && typeof b.label === 'string') {
return a.label.localeCompare(b.label);
}
if (typeof a.value === 'string' && typeof b.value === 'string') {
return a.value.localeCompare(b.value);
}
return (a.value as number) - (b.value as number);
};

/**
* It creates a comparator to check for a specific property.
* Can be used with string and number property values.
* */
export const propertyComparator = (property: string) => (
a: AntdLabeledValue,
b: AntdLabeledValue,
) => {
if (typeof a[property] === 'string' && typeof b[property] === 'string') {
return a[property].localeCompare(b[property]);
}
return (a[property] as number) - (b[property] as number);
};

const Select = ({
allowNewOptions = false,
ariaLabel,
Expand All @@ -189,6 +214,7 @@ const Select = ({
pageSize = DEFAULT_PAGE_SIZE,
placeholder = t('Select ...'),
showSearch = true,
sortComparator = defaultSortComparator,
value,
...props
}: SelectProps) => {
Expand Down Expand Up @@ -277,14 +303,21 @@ const Select = ({
}
});
}

const sortedOptions = [...topOptions, ...otherOptions];
const sortedOptions = [
...topOptions.sort(sortComparator),
...otherOptions.sort(sortComparator),
];
if (!isEqual(sortedOptions, selectOptions)) {
setSelectOptions(sortedOptions);
}
} else {
const sortedOptions = [...selectOptions].sort(sortComparator);
if (!isEqual(sortedOptions, selectOptions)) {
setSelectOptions(sortedOptions);
}
}
},
[isAsync, isSingleMode, labelInValue, selectOptions],
[isAsync, isSingleMode, labelInValue, selectOptions, sortComparator],
);

const handleOnSelect = (
Expand Down Expand Up @@ -342,28 +375,34 @@ const Select = ({
[onError],
);

const handleData = (data: OptionsType) => {
let mergedData: OptionsType = [];
if (data && Array.isArray(data) && data.length) {
const dataValues = new Set();
data.forEach(option =>
dataValues.add(String(option.value).toLocaleLowerCase()),
);

// merges with existing and creates unique options
setSelectOptions(prevOptions => {
mergedData = [
...prevOptions.filter(
previousOption =>
!dataValues.has(String(previousOption.value).toLocaleLowerCase()),
),
...data,
];
return mergedData;
});
}
return mergedData;
};
const handleData = useCallback(
(data: OptionsType) => {
let mergedData: OptionsType = [];
if (data && Array.isArray(data) && data.length) {
const dataValues = new Set();
data.forEach(option =>
dataValues.add(String(option.value).toLocaleLowerCase()),
);

// merges with existing and creates unique options
setSelectOptions(prevOptions => {
mergedData = [
...prevOptions.filter(
previousOption =>
!dataValues.has(
String(previousOption.value).toLocaleLowerCase(),
),
),
...data,
];
mergedData.sort(sortComparator);
return mergedData;
});
}
return mergedData;
},
[sortComparator],
);

const handlePaginatedFetch = useMemo(
() => (value: string, page: number, pageSize: number) => {
Expand Down Expand Up @@ -401,7 +440,7 @@ const Select = ({
setIsTyping(false);
});
},
[allValuesLoaded, fetchOnlyOnSearch, internalOnError, options],
[allValuesLoaded, fetchOnlyOnSearch, handleData, internalOnError, options],
);

const handleOnSearch = useMemo(
Expand Down Expand Up @@ -474,8 +513,8 @@ const Select = ({
}

// multiple or tags mode keep the dropdown visible while selecting options
// this waits for the dropdown to be closed before sorting the top options
if (!isSingleMode && !isDropdownVisible) {
// this waits for the dropdown to be opened before sorting the top options
if (!isSingleMode && isDropdownVisible) {
handleTopOptions(selectValue);
}
};
Expand Down
6 changes: 1 addition & 5 deletions superset-frontend/src/components/TableSelector/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,7 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
if (onTablesLoad) {
onTablesLoad(json.options);
}
setTableOptions(
options.sort((a: { text: string }, b: { text: string }) =>
a.text.localeCompare(b.text),
),
);
setTableOptions(options);
setCurrentTable(currentTable);
setLoadingTables(false);
})
Expand Down
15 changes: 9 additions & 6 deletions superset-frontend/src/components/TimezoneSelector/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,17 +83,13 @@ ALL_ZONES.forEach(zone => {
}
});

const TIMEZONE_OPTIONS = TIMEZONES.sort(
// sort by offset
(a, b) =>
moment.tz(currentDate, a.name).utcOffset() -
moment.tz(currentDate, b.name).utcOffset(),
).map(zone => ({
const TIMEZONE_OPTIONS = TIMEZONES.map(zone => ({
label: `GMT ${moment
.tz(currentDate, zone.name)
.format('Z')} (${getTimezoneName(zone.name)})`,
value: zone.name,
offsets: getOffsetKey(zone.name),
timezoneName: zone.name,
}));

const TimezoneSelector = ({ onTimezoneChange, timezone }: TimezoneProps) => {
Expand Down Expand Up @@ -126,6 +122,13 @@ const TimezoneSelector = ({ onTimezoneChange, timezone }: TimezoneProps) => {
onChange={onTimezoneChange}
value={timezone || DEFAULT_TIMEZONE.value}
options={TIMEZONE_OPTIONS}
sortComparator={(
a: typeof TIMEZONE_OPTIONS[number],
b: typeof TIMEZONE_OPTIONS[number],
) =>
moment.tz(currentDate, a.timezoneName).utcOffset() -
moment.tz(currentDate, b.timezoneName).utcOffset()
}
/>
);
};
Expand Down
Loading