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.

Sign up for GitHub

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

Jump to bottom

feat: Adds a helper text option to the Select component #21269

Merged
merged 3 commits into from
Sep 2, 2022
Merged

feat: Adds a helper text option to the Select component #21269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4bbfee
feat: Adds a helper text option to the Select component
michael-s-molina Aug 31, 2022
7075753
Fixes test description
michael-s-molina Sep 1, 2022
69edd74
Fixes test description
michael-s-molina Sep 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 30 additions & 44 deletions superset-frontend/src/components/Select/AsyncSelect.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,13 +206,7 @@ test('sort the options using a custom sort comparator', async () => {
option1: typeof OPTIONS[0],
option2: typeof OPTIONS[0],
) => option1.gender.localeCompare(option2.gender);
render(
<AsyncSelect
{...defaultProps}
options={loadOptions}
sortComparator={sortComparator}
/>,
);
render(<AsyncSelect {...defaultProps} sortComparator={sortComparator} />);
await open();
const options = await findAllSelectOptions();
const optionsPage = OPTIONS.slice(0, defaultProps.pageSize);
Expand Down Expand Up @@ -294,7 +288,7 @@ test('searches for label or value', async () => {
});

test('search order exact and startWith match first', async () => {
render(<AsyncSelect {...defaultProps} options={loadOptions} />);
render(<AsyncSelect {...defaultProps} />);
await open();
await type('Her');
expect(await findSelectOption('Guilherme')).toBeInTheDocument();
Expand Down Expand Up @@ -333,7 +327,7 @@ test('same case should be ranked to the top', async () => {
});

test('ignores special keys when searching', async () => {
render(<AsyncSelect {...defaultProps} options={loadOptions} />);
render(<AsyncSelect {...defaultProps} />);
await type('{shift}');
expect(screen.queryByText(LOADING)).not.toBeInTheDocument();
});
Expand Down Expand Up @@ -434,7 +428,7 @@ test('clear all the values', async () => {
});

test('does not add a new option if allowNewOptions is false', async () => {
render(<AsyncSelect {...defaultProps} options={loadOptions} />);
render(<AsyncSelect {...defaultProps} />);
await open();
await type(NEW_OPTION);
expect(await screen.findByText(NO_DATA)).toBeInTheDocument();
Expand Down Expand Up @@ -469,7 +463,7 @@ test('adds the null option when selected in multiple mode', async () => {
});

test('renders the select with default props', () => {
render(<AsyncSelect {...defaultProps} options={loadOptions} />);
render(<AsyncSelect {...defaultProps} />);
expect(getSelect()).toBeInTheDocument();
});

Expand All @@ -485,7 +479,7 @@ test('opens the select without any data', async () => {
});

test('displays the loading indicator when opening', async () => {
render(<AsyncSelect {...defaultProps} options={loadOptions} />);
render(<AsyncSelect {...defaultProps} />);
await waitFor(() => {
userEvent.click(getSelect());
expect(screen.getByText(LOADING)).toBeInTheDocument();
Expand All @@ -494,17 +488,15 @@ test('displays the loading indicator when opening', async () => {
});

test('makes a selection in single mode', async () => {
render(<AsyncSelect {...defaultProps} options={loadOptions} />);
render(<AsyncSelect {...defaultProps} />);
const optionText = 'Emma';
await open();
userEvent.click(await findSelectOption(optionText));
expect(await findSelectValue()).toHaveTextContent(optionText);
});

test('multiple selections in multiple mode', async () => {
render(
<AsyncSelect {...defaultProps} options={loadOptions} mode="multiple" />,
);
render(<AsyncSelect {...defaultProps} mode="multiple" />);
await open();
const [firstOption, secondOption] = OPTIONS;
userEvent.click(await findSelectOption(firstOption.label));
Expand All @@ -516,9 +508,7 @@ test('multiple selections in multiple mode', async () => {

test('changes the selected item in single mode', async () => {
const onChange = jest.fn();
render(
<AsyncSelect {...defaultProps} options={loadOptions} onChange={onChange} />,
);
render(<AsyncSelect {...defaultProps} onChange={onChange} />);
await open();
const [firstOption, secondOption] = OPTIONS;
userEvent.click(await findSelectOption(firstOption.label));
Expand All @@ -542,9 +532,7 @@ test('changes the selected item in single mode', async () => {
});

test('deselects an item in multiple mode', async () => {
render(
<AsyncSelect {...defaultProps} options={loadOptions} mode="multiple" />,
);
render(<AsyncSelect {...defaultProps} mode="multiple" />);
await open();
const option3 = OPTIONS[2];
const option8 = OPTIONS[7];
Expand Down Expand Up @@ -578,18 +566,14 @@ test('deselects an item in multiple mode', async () => {
});

test('adds a new option if none is available and allowNewOptions is true', async () => {
render(
<AsyncSelect {...defaultProps} options={loadOptions} allowNewOptions />,
);
render(<AsyncSelect {...defaultProps} allowNewOptions />);
await open();
await type(NEW_OPTION);
expect(await findSelectOption(NEW_OPTION)).toBeInTheDocument();
});

test('does not add a new option if the option already exists', async () => {
render(
<AsyncSelect {...defaultProps} options={loadOptions} allowNewOptions />,
);
render(<AsyncSelect {...defaultProps} allowNewOptions />);
const option = OPTIONS[0].label;
await open();
await type(option);
Expand All @@ -602,32 +586,21 @@ test('does not add a new option if the option already exists', async () => {
});

test('shows "No data" when allowNewOptions is false and a new option is entered', async () => {
render(
<AsyncSelect
{...defaultProps}
options={loadOptions}
allowNewOptions={false}
showSearch
/>,
);
render(<AsyncSelect {...defaultProps} allowNewOptions={false} showSearch />);
await open();
await type(NEW_OPTION);
expect(await screen.findByText(NO_DATA)).toBeInTheDocument();
});

test('does not show "No data" when allowNewOptions is true and a new option is entered', async () => {
render(
<AsyncSelect {...defaultProps} options={loadOptions} allowNewOptions />,
);
render(<AsyncSelect {...defaultProps} allowNewOptions />);
await open();
await type(NEW_OPTION);
expect(screen.queryByText(NO_DATA)).not.toBeInTheDocument();
});

test('sets a initial value in single mode', async () => {
render(
<AsyncSelect {...defaultProps} options={loadOptions} value={OPTIONS[0]} />,
);
render(<AsyncSelect {...defaultProps} value={OPTIONS[0]} />);
expect(await findSelectValue()).toHaveTextContent(OPTIONS[0].label);
});

Expand All @@ -636,7 +609,6 @@ test('sets a initial value in multiple mode', async () => {
<AsyncSelect
{...defaultProps}
mode="multiple"
options={loadOptions}
value={[OPTIONS[0], OPTIONS[1]]}
/>,
);
Expand All @@ -646,7 +618,7 @@ test('sets a initial value in multiple mode', async () => {
});

test('searches for matches in both loaded and unloaded pages', async () => {
render(<AsyncSelect {...defaultProps} options={loadOptions} />);
render(<AsyncSelect {...defaultProps} />);
await open();
await type('and');

Expand Down Expand Up @@ -750,6 +722,20 @@ test('fires a new request if all values have not been fetched', async () => {
expect(mock).toHaveBeenCalledTimes(2);
});

test('does not render a helper text by default', async () => {
render(<AsyncSelect {...defaultProps} />);
await open();
expect(screen.queryByRole('note')).not.toBeInTheDocument();
});

test('renders a helper text when one is provided', async () => {
const helperText = 'Helper text';
render(<AsyncSelect {...defaultProps} helperText={helperText} />);
await open();
expect(screen.getByRole('note')).toBeInTheDocument();
expect(screen.queryByText(helperText)).toBeInTheDocument();
});

/*
TODO: Add tests that require scroll interaction. Needs further investigation.
- Fetches more data when scrolling and more data is available
Expand Down
30 changes: 29 additions & 1 deletion superset-frontend/src/components/Select/AsyncSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ export interface AsyncSelectProps extends PickedSelectProps {
* Can be any ReactNode.
*/
header?: ReactNode;
/**
* It adds a helper text on top of the Select options
* with additional context to help with the interaction.
*/
helperText?: string;
/**
* It fires a request against the server after
* the first interaction and not on render.
Expand Down Expand Up @@ -182,6 +187,9 @@ const StyledSelect = styled(AntdSelect)`
.ant-select-arrow .anticon:not(.ant-select-suffix) {
pointer-events: none;
}
.ant-select-dropdown {
padding: 0;
}
`}
`;

Expand Down Expand Up @@ -224,6 +232,16 @@ const StyledLoadingText = styled.div`
`}
`;

const StyledHelperText = styled.div`
${({ theme }) => `
padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 3}px;
color: ${theme.colors.grayscale.base};
font-size: ${theme.typography.sizes.s}px;
cursor: default;
border-bottom: 1px solid ${theme.colors.grayscale.light2};
`}
`;

const MAX_TAG_COUNT = 4;
const TOKEN_SEPARATORS = [',', '\n', '\t', ';'];
const DEFAULT_PAGE_SIZE = 100;
Expand Down Expand Up @@ -297,6 +315,7 @@ const AsyncSelect = forwardRef(
fetchOnlyOnSearch,
filterOption = true,
header = null,
helperText,
invertSelection = false,
lazyLoading = true,
loading,
Expand Down Expand Up @@ -612,7 +631,16 @@ const AsyncSelect = forwardRef(
if (isLoading && fullSelectOptions.length === 0) {
return <StyledLoadingText>{t('Loading...')}</StyledLoadingText>;
}
return error ? <Error error={error} /> : originNode;
return error ? (
<Error error={error} />
) : (
<>
{helperText && (
<StyledHelperText role="note">{helperText}</StyledHelperText>
)}
{originNode}
</>
);
};

// use a function instead of component since every rerender of the
Expand Down
14 changes: 14 additions & 0 deletions superset-frontend/src/components/Select/Select.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,20 @@ test('triggers getPopupContainer if passed', async () => {
expect(getPopupContainer).toHaveBeenCalled();
});

test('does not render a helper text by default', async () => {
render(<Select {...defaultProps} />);
await open();
expect(screen.queryByRole('note')).not.toBeInTheDocument();
});

test('renders a helper text when one is provided', async () => {
const helperText = 'Helper text';
render(<Select {...defaultProps} helperText={helperText} />);
await open();
expect(screen.getByRole('note')).toBeInTheDocument();
expect(screen.queryByText(helperText)).toBeInTheDocument();
});

/*
TODO: Add tests that require scroll interaction. Needs further investigation.
- Fetches more data when scrolling and more data is available
Expand Down
28 changes: 27 additions & 1 deletion superset-frontend/src/components/Select/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ export interface SelectProps extends PickedSelectProps {
* Can be any ReactNode.
*/
header?: ReactNode;
/**
* It adds a helper text on top of the Select options
* with additional context to help with the interaction.
*/
helperText?: string;
/**
* It defines whether the Select should allow for the
* selection of multiple options or single.
Expand Down Expand Up @@ -139,6 +144,9 @@ const StyledSelect = styled(AntdSelect)`
.ant-select-arrow .anticon:not(.ant-select-suffix) {
pointer-events: none;
}
.ant-select-dropdown {
padding: 0;
}
`}
`;

Expand All @@ -162,6 +170,16 @@ const StyledLoadingText = styled.div`
`}
`;

const StyledHelperText = styled.div`
Copy link
Member

Choose a reason for hiding this comment

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

This looks identical to the AsyncSelect one. Should we put in a common file and import it from there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I left it duplicated on purpose in this PR because @cccs-RyanK is already working on the common file in #21094

${({ theme }) => `
padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 3}px;
color: ${theme.colors.grayscale.base};
font-size: ${theme.typography.sizes.s}px;
cursor: default;
border-bottom: 1px solid ${theme.colors.grayscale.light2};
`}
`;

const MAX_TAG_COUNT = 4;
const TOKEN_SEPARATORS = [',', '\n', '\t', ';'];
const EMPTY_OPTIONS: OptionsType = [];
Expand Down Expand Up @@ -224,6 +242,7 @@ const Select = forwardRef(
ariaLabel,
filterOption = true,
header = null,
helperText,
invertSelection = false,
labelInValue = false,
loading,
Expand Down Expand Up @@ -401,7 +420,14 @@ const Select = forwardRef(
if (isLoading && fullSelectOptions.length === 0) {
return <StyledLoadingText>{t('Loading...')}</StyledLoadingText>;
}
return originNode;
return (
<>
{helperText && (
<StyledHelperText role="note">{helperText}</StyledHelperText>
)}
{originNode}
</>
);
};

// use a function instead of component since every rerender of the
Expand Down