This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 5e64211bdb fix: Handles disabled options on Select All (#22830)
5e64211bdb is described below
commit 5e64211bdb0302315ee8f8e64f7a95180da594ad
Author: Michael S. Molina <[email protected]>
AuthorDate: Thu Feb 9 08:31:58 2023 -0500
fix: Handles disabled options on Select All (#22830)
---
.../src/components/Select/Select.test.tsx | 59 ++++++++++++++-
superset-frontend/src/components/Select/Select.tsx | 88 +++++++++++++---------
superset-frontend/src/components/Select/utils.tsx | 18 +++++
3 files changed, 128 insertions(+), 37 deletions(-)
diff --git a/superset-frontend/src/components/Select/Select.test.tsx
b/superset-frontend/src/components/Select/Select.test.tsx
index 220ad4fe98..f55299e3fc 100644
--- a/superset-frontend/src/components/Select/Select.test.tsx
+++ b/superset-frontend/src/components/Select/Select.test.tsx
@@ -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' },
@@ -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
diff --git a/superset-frontend/src/components/Select/Select.tsx
b/superset-frontend/src/components/Select/Select.tsx
index 9ab42fc20d..d7fe90d388 100644
--- a/superset-frontend/src/components/Select/Select.tsx
+++ b/superset-frontend/src/components/Select/Select.tsx
@@ -45,6 +45,8 @@ import {
getSuffixIcon,
SELECT_ALL_VALUE,
selectAllOption,
+ mapValues,
+ mapOptions,
} from './utils';
import { SelectOptionsType, SelectProps } from './types';
import {
@@ -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 = (
@@ -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)
@@ -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(
@@ -312,7 +336,7 @@ const Select = forwardRef(
);
const handleClear = () => {
- setSelectValue(undefined);
+ clear();
if (onClear) {
onClear();
}
@@ -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(
@@ -353,7 +377,7 @@ const Select = forwardRef(
value,
isSingleMode,
labelInValue,
- fullSelectOptions.length,
+ selectAllEligible.length,
selectOptions.length,
]);
@@ -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) => {
@@ -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);
diff --git a/superset-frontend/src/components/Select/utils.tsx
b/superset-frontend/src/components/Select/utils.tsx
index 67ecb4bde3..d6d352132f 100644
--- a/superset-frontend/src/components/Select/utils.tsx
+++ b/superset-frontend/src/components/Select/utils.tsx
@@ -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,
+ }));
