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

feat: Select all for synchronous select #22084

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0dfab9d
added select all button and a few tests
cccs-RyanK Oct 19, 2022
32e966d
Merge branch 'master' of github.com:CybercentreCanada/superset into s…
cccs-RyanK Nov 8, 2022
66af8c9
removed commented tests
cccs-RyanK Nov 8, 2022
6e7ac16
prettier fixes
cccs-RyanK Nov 8, 2022
4c599fd
lint fixes
cccs-RyanK Nov 10, 2022
06b77b0
added tests and feature for custom tag renderer
cccs-RyanK Nov 18, 2022
1efc58b
fixed onchange
cccs-RyanK Nov 23, 2022
42b45fd
Merge branch 'master' of github.com:CybercentreCanada/superset into s…
cccs-RyanK Nov 28, 2022
14129d0
fixed value format
cccs-RyanK Nov 29, 2022
3b596c6
Merge branch 'master' of github.com:CybercentreCanada/superset into s…
cccs-RyanK Nov 29, 2022
16e3c66
fixing wrong type issues
cccs-RyanK Nov 30, 2022
c69b7af
select test fixes
cccs-RyanK Nov 30, 2022
3d120d1
SqlEditor test changed with new select all behavior
cccs-RyanK Nov 30, 2022
6ca115e
fixed some lint errors
cccs-RyanK Dec 1, 2022
5702dd4
Merge branch 'master' of github.com:CybercentreCanada/superset into s…
cccs-RyanK Dec 14, 2022
c6c5f55
feedback suggestions
cccs-RyanK Dec 20, 2022
e36ceed
Merge branch 'master' of github.com:CybercentreCanada/superset into s…
cccs-RyanK Dec 20, 2022
426f57c
added tag renderer to async select
cccs-RyanK Dec 20, 2022
88f125e
fixed hook dependencies and ts error
Jan 12, 2023
807f51c
Merge branch 'master' of github.com:CybercentreCanada/superset into s…
Jan 12, 2023
618161f
removed custom max tag placeholder from select filter plugin
Jan 13, 2023
6b9b428
prettier fixes
Jan 16, 2023
0d66466
fixed bug in maxTagPlaceholder and value filter plugin tests
Jan 17, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -404,9 +404,9 @@ describe('Horizontal FilterBar', () => {
saveNativeFilterSettings([SAMPLE_CHART]);
cy.getBySel('filter-bar').within(() => {
cy.get(nativeFilters.filterItem).contains('Albania').should('be.visible');
cy.get(nativeFilters.filterItem).contains('+1').should('be.visible');
cy.get(nativeFilters.filterItem).contains('+ 1 ...').should('be.visible');
cy.get('.ant-select-selection-search-input').click();
cy.get(nativeFilters.filterItem).contains('+2').should('be.visible');
cy.get(nativeFilters.filterItem).contains('+ 2 ...').should('be.visible');
});
});
});
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/components/Select/AsyncSelect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ import {
TOKEN_SEPARATORS,
DEFAULT_SORT_COMPARATOR,
} from './constants';
import { oneLineTagRender } from './CustomTag';
import { customTagRender } from './CustomTag';

const Error = ({ error }: { error: string }) => (
<StyledError>
Expand Down Expand Up @@ -517,7 +517,7 @@ const AsyncSelect = forwardRef(
)
}
oneLine={oneLine}
tagRender={oneLine ? oneLineTagRender : undefined}
tagRender={customTagRender}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we only used a custom tag renderer if oneLine was true. Does customTagRender preserve the same behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes customTagRender is the same as oneLineTagRender. It has the same behaviour except it does not render the select all tag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean to ask is if customTagRender has the same behavior as tagRender={undefined}

Copy link
Contributor Author

@cccs-RyanK cccs-RyanK Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. The only difference between customTagRender and the default is the inclusion of the onMouseDown handler that prevents the dropdown from opening when clicking the tag icons. This was added for the oneLine mode and it seemed useful for the regular use case, but i can add a check to not include it when the component is not in oneLine mode?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I like the behavior. No change is needed then.

{...props}
ref={ref}
>
Expand Down
21 changes: 13 additions & 8 deletions superset-frontend/src/components/Select/CustomTag.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import { styled } from '@superset-ui/core';
import { useCSSTextTruncation } from 'src/hooks/useTruncation';
import { Tooltip } from '../Tooltip';
import { CustomTagProps } from './types';
import { SELECT_ALL_VALUE } from './utils';
import { NoElement } from './styles';

const StyledTag = styled(AntdTag)`
& .ant-tag-close-icon {
Expand Down Expand Up @@ -51,10 +53,10 @@ const Tag = (props: any) => {
};

/**
* Custom tag renderer dedicated for oneLine mode
* Custom tag renderer
*/
export const oneLineTagRender = (props: CustomTagProps) => {
const { label } = props;
export const customTagRender = (props: CustomTagProps) => {
const { label, value } = props;

const onPreventMouseDown = (event: React.MouseEvent<HTMLElement>) => {
// if close icon is clicked, stop propagation to avoid opening the dropdown
Expand All @@ -69,9 +71,12 @@ export const oneLineTagRender = (props: CustomTagProps) => {
}
};

return (
<Tag onMouseDown={onPreventMouseDown} {...props}>
{label}
</Tag>
);
if (value !== SELECT_ALL_VALUE) {
return (
<Tag onMouseDown={onPreventMouseDown} {...(props as object)}>
{label}
</Tag>
);
}
return <NoElement />;
};
8 changes: 8 additions & 0 deletions superset-frontend/src/components/Select/Select.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,13 @@ const ARG_TYPES = {
Requires '"mode=multiple"'.
`,
},
maxTagCount: {
defaultValue: 4,
description: `Sets maxTagCount attribute. The overflow tag is displayed in
place of the remaining items.
Requires '"mode=multiple"'.
`,
},
};

const mountHeader = (type: String) => {
Expand Down Expand Up @@ -207,6 +214,7 @@ InteractiveSelect.args = {
placeholder: 'Select ...',
optionFilterProps: ['value', 'label', 'custom'],
oneLine: false,
maxTagCount: 4,
};

InteractiveSelect.argTypes = {
Expand Down
225 changes: 215 additions & 10 deletions superset-frontend/src/components/Select/Select.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
import React from 'react';
import { render, screen, waitFor, within } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import { Select } from 'src/components';
import Select from 'src/components/Select/Select';
import { SELECT_ALL_VALUE } from './utils';

const ARIA_LABEL = 'Test';
const NEW_OPTION = 'Kyle';
Expand Down Expand Up @@ -64,6 +65,9 @@ const defaultProps = {
showSearch: true,
};

const selectAllOptionLabel = (numOptions: number) =>
`${String(SELECT_ALL_VALUE)} (${numOptions})`;

const getElementByClassName = (className: string) =>
document.querySelector(className)! as HTMLElement;

Expand All @@ -89,7 +93,12 @@ const findSelectValue = () =>
waitFor(() => getElementByClassName('.ant-select-selection-item'));

const findAllSelectValues = () =>
waitFor(() => getElementsByClassName('.ant-select-selection-item'));
waitFor(() => [...getElementsByClassName('.ant-select-selection-item')]);

const findAllCheckedValues = () =>
waitFor(() => [
...getElementsByClassName('.ant-select-item-option-selected'),
]);

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

Expand Down Expand Up @@ -209,26 +218,37 @@ test('should sort selected to the top when in multi mode', async () => {
let labels = originalLabels.slice();

await open();
userEvent.click(await findSelectOption(labels[1]));
expect(await matchOrder(labels)).toBe(true);
userEvent.click(await findSelectOption(labels[2]));
expect(
await matchOrder([selectAllOptionLabel(originalLabels.length), ...labels]),
).toBe(true);

await type('{esc}');
await open();
labels = labels.splice(1, 1).concat(labels);
expect(await matchOrder(labels)).toBe(true);
labels = labels.splice(2, 1).concat(labels);
expect(
await matchOrder([selectAllOptionLabel(originalLabels.length), ...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);
expect(
await matchOrder([selectAllOptionLabel(originalLabels.length), ...labels]),
).toBe(true);

// should revert to original order
clearAll();
await type('{esc}');
await open();
expect(await matchOrder(originalLabels)).toBe(true);
expect(
await matchOrder([
selectAllOptionLabel(originalLabels.length),
...originalLabels,
]),
).toBe(true);
});

test('searches for label or value', async () => {
Expand Down Expand Up @@ -440,15 +460,15 @@ test('changes the selected item in single mode', async () => {
label: firstOption.label,
value: firstOption.value,
}),
firstOption,
expect.objectContaining(firstOption),
);
userEvent.click(await findSelectOption(secondOption.label));
expect(onChange).toHaveBeenCalledWith(
expect.objectContaining({
label: secondOption.label,
value: secondOption.value,
}),
secondOption,
expect.objectContaining(secondOption),
);
expect(await findSelectValue()).toHaveTextContent(secondOption.label);
});
Expand Down Expand Up @@ -566,6 +586,136 @@ test('finds an element with a numeric value and does not duplicate the options',
expect(await querySelectOption('11')).not.toBeInTheDocument();
});

test('render "Select all" for multi select', async () => {
render(<Select {...defaultProps} mode="multiple" options={OPTIONS} />);
await open();
const options = await findAllSelectOptions();
expect(options[0]).toHaveTextContent(selectAllOptionLabel(OPTIONS.length));
});

test('does not render "Select all" for single select', async () => {
render(<Select {...defaultProps} options={OPTIONS} mode="single" />);
await open();
expect(
screen.queryByText(selectAllOptionLabel(OPTIONS.length)),
).not.toBeInTheDocument();
});

test('does not render "Select all" for an empty multiple select', async () => {
render(<Select {...defaultProps} options={[]} mode="multiple" />);
await open();
expect(
screen.queryByText(selectAllOptionLabel(OPTIONS.length)),
).not.toBeInTheDocument();
});

test('does not render "Select all" when searching', async () => {
render(<Select {...defaultProps} options={OPTIONS} mode="multiple" />);
await open();
await type('Select');
expect(
screen.queryByText(selectAllOptionLabel(OPTIONS.length)),
).not.toBeInTheDocument();
});

test('does not render "Select all" as one of the tags after selection', async () => {
render(<Select {...defaultProps} options={OPTIONS} mode="multiple" />);
await open();
userEvent.click(await findSelectOption(selectAllOptionLabel(OPTIONS.length)));
const values = await findAllSelectValues();
expect(values[0]).not.toHaveTextContent(selectAllOptionLabel(OPTIONS.length));
});

test('keeps "Select all" at the top after a selection', async () => {
const selected = OPTIONS[2];
render(
<Select
{...defaultProps}
options={OPTIONS.slice(0, 10)}
mode="multiple"
value={[selected]}
/>,
);
await open();
const options = await findAllSelectOptions();
expect(options[0]).toHaveTextContent(selectAllOptionLabel(10));
expect(options[1]).toHaveTextContent(selected.label);
});

test('selects all values', async () => {
render(
<Select
{...defaultProps}
options={OPTIONS}
mode="multiple"
maxTagCount={0}
/>,
);
await open();
userEvent.click(await findSelectOption(selectAllOptionLabel(OPTIONS.length)));
const values = await findAllSelectValues();
expect(values.length).toBe(1);
expect(values[0]).toHaveTextContent(`+ ${OPTIONS.length} ...`);
});

test('unselects all values', async () => {
render(
<Select
{...defaultProps}
options={OPTIONS}
mode="multiple"
maxTagCount={0}
/>,
);
await open();
userEvent.click(await findSelectOption(selectAllOptionLabel(OPTIONS.length)));
let values = await findAllSelectValues();
expect(values.length).toBe(1);
expect(values[0]).toHaveTextContent(`+ ${OPTIONS.length} ...`);
userEvent.click(await findSelectOption(selectAllOptionLabel(OPTIONS.length)));
values = await findAllSelectValues();
expect(values.length).toBe(0);
});

test('deselecting a value also deselects "Select all"', async () => {
render(
<Select
{...defaultProps}
options={OPTIONS.slice(0, 10)}
mode="multiple"
maxTagCount={0}
/>,
);
await open();
userEvent.click(await findSelectOption(selectAllOptionLabel(10)));
let values = await findAllCheckedValues();
expect(values[0]).toHaveTextContent(selectAllOptionLabel(10));
userEvent.click(await findSelectOption(OPTIONS[0].label));
values = await findAllCheckedValues();
expect(values[0]).not.toHaveTextContent(selectAllOptionLabel(10));
});

test('selecting all values also selects "Select all"', async () => {
render(
<Select
{...defaultProps}
options={OPTIONS.slice(0, 10)}
mode="multiple"
maxTagCount={0}
/>,
);
await open();
const options = await findAllSelectOptions();
options.forEach((option, index) => {
// skip select all
if (index > 0) {
userEvent.click(option);
}
});
const values = await findAllSelectValues();
expect(values[0]).toHaveTextContent(`+ 10 ...`);
});

test('Renders only 1 tag and an overflow tag in oneLine mode', () => {
render(
<Select
Expand Down Expand Up @@ -614,6 +764,61 @@ test('Renders only an overflow tag if dropdown is open in oneLine mode', async (
expect(withinSelector.getByText('+ 2 ...')).toBeVisible();
});

test('+N tag does not count the "Select All" option', async () => {
render(
<Select
{...defaultProps}
options={OPTIONS.slice(0, 10)}
mode="multiple"
maxTagCount={0}
/>,
);
await open();
userEvent.click(await findSelectOption(selectAllOptionLabel(10)));
const values = await findAllSelectValues();
// maxTagCount is 0 so the +N tag should be + 10 ...
expect(values[0]).toHaveTextContent('+ 10 ...');
});

test('"Select All" is checked when unchecking a newly added option and all the other options are still selected', async () => {
render(
<Select
{...defaultProps}
options={OPTIONS.slice(0, 10)}
mode="multiple"
allowNewOptions
/>,
);
await open();
userEvent.click(await findSelectOption(selectAllOptionLabel(10)));
expect(await findSelectOption(selectAllOptionLabel(10))).toBeInTheDocument();
// add a new option
await type(`${NEW_OPTION}{enter}`);
expect(await findSelectOption(selectAllOptionLabel(11))).toBeInTheDocument();
expect(await findSelectOption(NEW_OPTION)).toBeInTheDocument();
// select all should be selected
let values = await findAllCheckedValues();
expect(values[0]).toHaveTextContent(selectAllOptionLabel(11));
// remove new option
userEvent.click(await findSelectOption(NEW_OPTION));
// select all should still be selected
values = await findAllCheckedValues();
expect(values[0]).toHaveTextContent(selectAllOptionLabel(10));
expect(await findSelectOption(selectAllOptionLabel(10))).toBeInTheDocument();
});

test('does not render "Select All" when there are 0 or 1 options', async () => {
render(
<Select {...defaultProps} options={[]} mode="multiple" allowNewOptions />,
);
await open();
expect(screen.queryByText(selectAllOptionLabel(0))).not.toBeInTheDocument();
await type(`${NEW_OPTION}{enter}`);
expect(screen.queryByText(selectAllOptionLabel(1))).not.toBeInTheDocument();
await type(`Kyle2{enter}`);
expect(screen.queryByText(selectAllOptionLabel(2))).toBeInTheDocument();
});

/*
TODO: Add tests that require scroll interaction. Needs further investigation.
- Fetches more data when scrolling and more data is available
Expand Down
Loading