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: Handles disabled options on Select All #22830

Merged
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
59 changes: 58 additions & 1 deletion superset-frontend/src/components/Select/Select.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,18 @@ import userEvent from '@testing-library/user-event';
import Select from 'src/components/Select/Select';
import { SELECT_ALL_VALUE } from './utils';

type Option = {
label: string;
value: number;
gender: string;
disabled?: boolean;
};

const ARIA_LABEL = 'Test';
const NEW_OPTION = 'Kyle';
const NO_DATA = 'No Data';
const LOADING = 'Loading...';
const OPTIONS = [
const OPTIONS: Option[] = [
{ label: 'John', value: 1, gender: 'Male' },
{ label: 'Liam', value: 2, gender: 'Male' },
{ label: 'Olivia', value: 3, gender: 'Female' },
Expand Down Expand Up @@ -826,6 +833,56 @@ test('does not render "Select All" when there are 0 or 1 options', async () => {
expect(screen.queryByText(selectAllOptionLabel(2))).toBeInTheDocument();
});

test('do not count unselected disabled options in "Select All"', async () => {
const options = [...OPTIONS];
options[0].disabled = true;
options[1].disabled = true;
render(
<Select
{...defaultProps}
options={options}
mode="multiple"
value={options[0]}
/>,
);
await open();
// We have 2 options disabled but one is selected initially
// Select All should count one and ignore the other
expect(
screen.getByText(selectAllOptionLabel(OPTIONS.length - 1)),
).toBeInTheDocument();
});

test('"Select All" does not affect disabled options', async () => {
const options = [...OPTIONS];
options[0].disabled = true;
options[1].disabled = true;
render(
<Select
{...defaultProps}
options={options}
mode="multiple"
value={options[0]}
/>,
);
await open();

// We have 2 options disabled but one is selected initially
expect(await findSelectValue()).toHaveTextContent(options[0].label);
expect(await findSelectValue()).not.toHaveTextContent(options[1].label);

// Checking Select All shouldn't affect the disabled options
const selectAll = selectAllOptionLabel(OPTIONS.length - 1);
userEvent.click(await findSelectOption(selectAll));
expect(await findSelectValue()).toHaveTextContent(options[0].label);
expect(await findSelectValue()).not.toHaveTextContent(options[1].label);

// Unchecking Select All shouldn't affect the disabled options
userEvent.click(await findSelectOption(selectAll));
expect(await findSelectValue()).toHaveTextContent(options[0].label);
expect(await findSelectValue()).not.toHaveTextContent(options[1].label);
});

/*
TODO: Add tests that require scroll interaction. Needs further investigation.
- Fetches more data when scrolling and more data is available
Expand Down
88 changes: 52 additions & 36 deletions superset-frontend/src/components/Select/Select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ import {
getSuffixIcon,
SELECT_ALL_VALUE,
selectAllOption,
mapValues,
mapOptions,
} from './utils';
import { SelectOptionsType, SelectProps } from './types';
import {
Expand Down Expand Up @@ -177,23 +179,31 @@ const Select = forwardRef(
return result.filter(opt => opt.value !== SELECT_ALL_VALUE);
}, [selectOptions, selectValue]);

const enabledOptions = useMemo(
() => fullSelectOptions.filter(option => !option.disabled),
[fullSelectOptions],
);

const selectAllEligible = useMemo(
() =>
fullSelectOptions.filter(
option => hasOption(option.value, selectValue) || !option.disabled,
),
[fullSelectOptions, selectValue],
);

const selectAllEnabled = useMemo(
() =>
!isSingleMode &&
selectOptions.length > 0 &&
fullSelectOptions.length > 1 &&
enabledOptions.length > 1 &&
!inputValue,
[
isSingleMode,
selectOptions.length,
fullSelectOptions.length,
inputValue,
],
[isSingleMode, selectOptions.length, enabledOptions.length, inputValue],
);

const selectAllMode = useMemo(
() => ensureIsArray(selectValue).length === fullSelectOptions.length + 1,
[selectValue, fullSelectOptions],
() => ensureIsArray(selectValue).length === selectAllEligible.length + 1,
[selectValue, selectAllEligible],
);

const handleOnSelect = (
Expand All @@ -209,19 +219,19 @@ const Select = forwardRef(
if (value === getValue(SELECT_ALL_VALUE)) {
if (isLabeledValue(selectedItem)) {
return [
...fullSelectOptions,
...selectAllEligible,
selectAllOption,
] as AntdLabeledValue[];
}
return [
SELECT_ALL_VALUE,
...fullSelectOptions.map(opt => opt.value),
...selectAllEligible.map(opt => opt.value),
] as AntdLabeledValue[];
}
if (!hasOption(value, array)) {
const result = [...array, selectedItem];
if (
result.length === fullSelectOptions.length &&
result.length === selectAllEligible.length &&
selectAllEnabled
) {
return isLabeledValue(selectedItem)
Expand All @@ -236,12 +246,26 @@ const Select = forwardRef(
setInputValue('');
};

const clear = () => {
setSelectValue(
fullSelectOptions
.filter(
option => option.disabled && hasOption(option.value, selectValue),
)
.map(option =>
labelInValue
? { label: option.label, value: option.value }
: option.value,
),
);
};

const handleOnDeselect = (
value: string | number | AntdLabeledValue | undefined,
) => {
if (Array.isArray(selectValue)) {
if (getValue(value) === getValue(SELECT_ALL_VALUE)) {
setSelectValue(undefined);
clear();
} else {
let array = selectValue as AntdLabeledValue[];
array = array.filter(
Expand Down Expand Up @@ -312,7 +336,7 @@ const Select = forwardRef(
);

const handleClear = () => {
setSelectValue(undefined);
clear();
if (onClear) {
onClear();
}
Expand All @@ -337,7 +361,7 @@ const Select = forwardRef(
// if all values are selected, add select all to value
if (
!isSingleMode &&
ensureIsArray(value).length === fullSelectOptions.length &&
ensureIsArray(value).length === selectAllEligible.length &&
selectOptions.length > 0
) {
setSelectValue(
Expand All @@ -353,7 +377,7 @@ const Select = forwardRef(
value,
isSingleMode,
labelInValue,
fullSelectOptions.length,
selectAllEligible.length,
selectOptions.length,
]);

Expand All @@ -362,21 +386,21 @@ const Select = forwardRef(
v => getValue(v) === SELECT_ALL_VALUE,
);
if (checkSelectAll && !selectAllMode) {
const optionsToSelect = fullSelectOptions.map(option =>
const optionsToSelect = selectAllEligible.map(option =>
labelInValue ? option : option.value,
);
optionsToSelect.push(labelInValue ? selectAllOption : SELECT_ALL_VALUE);
setSelectValue(optionsToSelect);
}
}, [selectValue, selectAllMode, labelInValue, fullSelectOptions]);
}, [selectValue, selectAllMode, labelInValue, selectAllEligible]);

const selectAllLabel = useMemo(
() => () =>
`${SELECT_ALL_VALUE} (${formatNumber(
NumberFormats.INTEGER,
fullSelectOptions.length,
selectAllEligible.length,
)})`,
[fullSelectOptions.length],
[selectAllEligible],
);

const handleOnChange = (values: any, options: any) => {
Expand All @@ -393,30 +417,22 @@ const Select = forwardRef(
) {
// send all options to onchange if all are not currently there
if (!selectAllMode) {
newValues = labelInValue
? fullSelectOptions.map(opt => ({
key: opt.value,
value: opt.value,
label: opt.label,
}))
: fullSelectOptions.map(opt => opt.value);
newOptions = fullSelectOptions.map(opt => ({
children: opt.label,
key: opt.value,
value: opt.value,
label: opt.label,
}));
newValues = mapValues(selectAllEligible, labelInValue);
newOptions = mapOptions(selectAllEligible);
} else {
newValues = ensureIsArray(values).filter(
(val: any) => getValue(val) !== SELECT_ALL_VALUE,
);
}
} else if (
ensureIsArray(values).length === fullSelectOptions.length &&
ensureIsArray(values).length === selectAllEligible.length &&
selectAllMode
) {
newValues = [];
newValues = [];
const array = selectAllEligible.filter(
option => hasOption(option.value, selectValue) && option.disabled,
);
newValues = mapValues(array, labelInValue);
newOptions = mapOptions(array);
}
}
onChange?.(newValues, newOptions);
Expand Down
18 changes: 18 additions & 0 deletions superset-frontend/src/components/Select/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -204,3 +204,21 @@ export const renderSelectOptions = (options: SelectOptionsType) =>
</Option>
);
});

export const mapValues = (values: SelectOptionsType, labelInValue: boolean) =>
labelInValue
? values.map(opt => ({
key: opt.value,
value: opt.value,
label: opt.label,
}))
: values.map(opt => opt.value);

export const mapOptions = (values: SelectOptionsType) =>
values.map(opt => ({
children: opt.label,
key: opt.value,
value: opt.value,
label: opt.label,
disabled: opt.disabled,
}));