Skip to content

Commit

Permalink
fix: Handles disabled options on Select All (apache#22830)
Browse files Browse the repository at this point in the history
  • Loading branch information
michael-s-molina authored and PawankumarES committed Feb 13, 2023
1 parent ae42464 commit 823c013
Show file tree
Hide file tree
Showing 3 changed files with 128 additions and 37 deletions.
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,22 +386,22 @@ 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(
() => () =>
// TODO: localize
`${SELECT_ALL_VALUE} (${formatNumber(
NumberFormats.INTEGER,
fullSelectOptions.length,
selectAllEligible.length,
)})`,
[fullSelectOptions.length],
[selectAllEligible],
);

const handleOnChange = (values: any, options: any) => {
Expand All @@ -394,30 +418,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,
}));

0 comments on commit 823c013

Please sign in to comment.