New Select: Try it out with some core components (#92826)

* Export Combobox and add portalling

* Use floatingui fixed strategy and fix z-index

* Check non null

* Make value string type only

* Try with fiscal year setting

* Use combobox for WeekStartPicker

* Improve screen reader handling

* Fix faulty import

* Fix type issue

* Fix failing tests and export unstable

* Rename option and remove export

* Use comboboxMockSetup function

* Add support for number type

* menuClosed styles to emotion
This commit is contained in:
Tobias Skarhed
2024-09-10 16:28:00 +02:00
committed by GitHub
parent 2c93120a42
commit 096105fed6
12 changed files with 180 additions and 83 deletions

View File

@ -595,6 +595,10 @@ exports[`better eslint`] = {
[0, 0, 0, "No untranslated strings. Wrap text with <Trans />", "0"],
[0, 0, 0, "No untranslated strings. Wrap text with <Trans />", "1"]
],
"packages/grafana-ui/src/components/Combobox/Combobox.tsx:5381": [
[0, 0, 0, "Do not use any type assertions.", "0"],
[0, 0, 0, "Do not use any type assertions.", "1"]
],
"packages/grafana-ui/src/components/ConfirmButton/ConfirmButton.tsx:5381": [
[0, 0, 0, "No untranslated strings. Wrap text with <Trans />", "0"]
],

View File

@ -6,7 +6,7 @@ import React, { ComponentProps, useEffect, useState } from 'react';
import { Alert } from '../Alert/Alert';
import { Field } from '../Forms/Field';
import { Combobox, Option, Value } from './Combobox';
import { Combobox, ComboboxOption } from './Combobox';
const chance = new Chance();
@ -69,7 +69,7 @@ type Story = StoryObj<typeof Combobox>;
export const Basic: Story = {};
async function generateOptions(amount: number): Promise<Option[]> {
async function generateOptions(amount: number): Promise<ComboboxOption[]> {
return Array.from({ length: amount }, (_, index) => ({
label: chance.sentence({ words: index % 5 }),
value: chance.guid(),
@ -78,8 +78,8 @@ async function generateOptions(amount: number): Promise<Option[]> {
}
const ManyOptionsStory: StoryFn<PropsAndCustomArgs> = ({ numberOfOptions, ...args }) => {
const [value, setValue] = useState<Value | null>(null);
const [options, setOptions] = useState<Option[]>([]);
const [value, setValue] = useState<string | null>(null);
const [options, setOptions] = useState<ComboboxOption[]>([]);
const [isLoading, setIsLoading] = useState(true);
useEffect(() => {

View File

@ -1,10 +1,10 @@
import { render, screen, fireEvent } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { Combobox, Option } from './Combobox';
import { Combobox, ComboboxOption } from './Combobox';
// Mock data for the Combobox options
const options: Option[] = [
const options: ComboboxOption[] = [
{ label: 'Option 1', value: '1' },
{ label: 'Option 2', value: '2' },
{ label: 'Option 3', value: '3', description: 'This is option 3' },
@ -73,9 +73,10 @@ describe('Combobox', () => {
const input = screen.getByRole('combobox');
await userEvent.click(input);
await userEvent.keyboard('{ArrowDown}{ArrowDown}{Enter}');
expect(onChangeHandler).toHaveBeenCalledWith(options[1]);
expect(screen.queryByDisplayValue('Option 2')).toBeInTheDocument();
await userEvent.keyboard('{ArrowDown}{ArrowDown}{Enter}'); // Focus is at index 0 to start with
expect(onChangeHandler).toHaveBeenCalledWith(options[2]);
expect(screen.queryByDisplayValue('Option 3')).toBeInTheDocument();
});
it('clears selected value', async () => {
@ -91,4 +92,44 @@ describe('Combobox', () => {
expect(onChangeHandler).toHaveBeenCalledWith(null);
expect(screen.queryByDisplayValue('Option 2')).not.toBeInTheDocument();
});
describe('create custom value', () => {
it('should allow creating a custom value', async () => {
const onChangeHandler = jest.fn();
render(<Combobox options={options} value={null} onChange={onChangeHandler} createCustomValue />);
const input = screen.getByRole('combobox');
await userEvent.type(input, 'custom value');
await userEvent.keyboard('{Enter}');
expect(screen.getByDisplayValue('custom value')).toBeInTheDocument();
expect(onChangeHandler).toHaveBeenCalledWith(
expect.objectContaining({ label: 'custom value', value: 'custom value' })
);
});
it('should proivde custom string when all options are numbers', async () => {
const options = [
{ label: '1', value: 1 },
{ label: '2', value: 2 },
{ label: '3', value: 3 },
];
const onChangeHandler = jest.fn();
render(<Combobox options={options} value={null} onChange={onChangeHandler} createCustomValue />);
const input = screen.getByRole('combobox');
await userEvent.type(input, 'custom value');
await userEvent.keyboard('{Enter}');
expect(screen.getByDisplayValue('custom value')).toBeInTheDocument();
expect(typeof onChangeHandler.mock.calls[0][0].value === 'string').toBeTruthy();
expect(typeof onChangeHandler.mock.calls[0][0].value === 'number').toBeFalsy();
await userEvent.click(input);
await userEvent.keyboard('{Enter}'); // Select 1 as the first option
expect(typeof onChangeHandler.mock.calls[1][0].value === 'string').toBeFalsy();
expect(typeof onChangeHandler.mock.calls[1][0].value === 'number').toBeTruthy();
});
});
});

View File

@ -2,7 +2,7 @@ import { cx } from '@emotion/css';
import { autoUpdate, flip, size, useFloating } from '@floating-ui/react';
import { useVirtualizer } from '@tanstack/react-virtual';
import { useCombobox } from 'downshift';
import { SetStateAction, useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { SetStateAction, useCallback, useEffect, useId, useMemo, useRef, useState } from 'react';
import { useStyles2 } from '../../themes';
import { t } from '../../utils/i18n';
@ -11,30 +11,29 @@ import { Input, Props as InputProps } from '../Input/Input';
import { getComboboxStyles } from './getComboboxStyles';
export type Value = string | number;
export type Option = {
export type ComboboxOption<T extends string | number = string> = {
label: string;
value: Value;
value: T;
description?: string;
};
interface ComboboxProps
interface ComboboxProps<T extends string | number>
extends Omit<InputProps, 'prefix' | 'suffix' | 'value' | 'addonBefore' | 'addonAfter' | 'onChange'> {
onChange: (val: Option | null) => void;
value: Value | null;
options: Option[];
isClearable?: boolean;
createCustomValue?: boolean;
options: Array<ComboboxOption<T>>;
onChange: (option: ComboboxOption<T> | null) => void;
value: T | null;
}
function itemToString(item: Option | null) {
return item?.label ?? item?.value?.toString() ?? '';
function itemToString(item: ComboboxOption<string | number> | null) {
return item?.label ?? item?.value.toString() ?? '';
}
function itemFilter(inputValue: string) {
function itemFilter<T extends string | number>(inputValue: string) {
const lowerCasedInputValue = inputValue.toLowerCase();
return (item: Option) => {
return (item: ComboboxOption<T>) => {
return (
!inputValue ||
item?.label?.toLowerCase().includes(lowerCasedInputValue) ||
@ -53,15 +52,21 @@ const INDEX_WIDTH_CALCULATION = 100;
// A multiplier guesstimate times the amount of characters. If any padding or image support etc. is added this will need to be updated.
const WIDTH_MULTIPLIER = 7.3;
export const Combobox = ({
/**
* A performant Select replacement.
*
* @alpha
*/
export const Combobox = <T extends string | number>({
options,
onChange,
value,
isClearable = false,
createCustomValue = false,
id,
'aria-labelledby': ariaLabelledBy,
...restProps
}: ComboboxProps) => {
}: ComboboxProps<T>) => {
const [items, setItems] = useState(options);
const selectedItemIndex = useMemo(() => {
@ -78,7 +83,7 @@ export const Combobox = ({
}, [options, value]);
const selectedItem = useMemo(() => {
if (selectedItemIndex) {
if (selectedItemIndex !== null) {
return options[selectedItemIndex];
}
@ -95,6 +100,9 @@ export const Combobox = ({
const inputRef = useRef<HTMLInputElement>(null);
const floatingRef = useRef<HTMLDivElement>(null);
const menuId = `downshift-${useId().replace(/:/g, '--')}-menu`;
const labelId = `downshift-${useId().replace(/:/g, '--')}-label`;
const styles = useStyles2(getComboboxStyles);
const [popoverMaxWidth, setPopoverMaxWidth] = useState<number | undefined>(undefined);
const [popoverWidth, setPopoverWidth] = useState<number | undefined>(undefined);
@ -119,26 +127,28 @@ export const Combobox = ({
closeMenu,
selectItem,
} = useCombobox({
menuId,
labelId,
inputId: id,
items,
itemToString,
selectedItem,
onSelectedItemChange: ({ selectedItem, inputValue }) => {
onSelectedItemChange: ({ selectedItem }) => {
onChange(selectedItem);
},
defaultHighlightedIndex: selectedItemIndex ?? undefined,
defaultHighlightedIndex: selectedItemIndex ?? 0,
scrollIntoView: () => {},
onInputValueChange: ({ inputValue }) => {
const filteredItems = options.filter(itemFilter(inputValue));
if (createCustomValue && inputValue && filteredItems.findIndex((opt) => opt.label === inputValue) === -1) {
setItems([
...filteredItems,
{
label: inputValue,
value: inputValue,
description: t('combobox.custom-value.create', 'Create custom value'),
},
]);
const customValueOption: ComboboxOption<T> = {
label: inputValue,
// @ts-ignore Type casting needed to make this work when T is a number
value: inputValue as unknown as T,
description: t('combobox.custom-value.create', 'Create custom value'),
};
setItems([...filteredItems, customValueOption]);
return;
} else {
setItems(filteredItems);
@ -177,6 +187,7 @@ export const Combobox = ({
];
const elements = { reference: inputRef.current, floating: floatingRef.current };
const { floatingStyles } = useFloating({
strategy: 'fixed',
open: isOpen,
placement: 'bottom-start',
middleware,
@ -231,17 +242,21 @@ export const Combobox = ({
*/
onChange: () => {},
onBlur,
'aria-labelledby': ariaLabelledBy, // Label should be handled with the Field component
})}
/>
<div
className={cx(styles.menu, hasMinHeight && styles.menuHeight)}
className={cx(styles.menu, hasMinHeight && styles.menuHeight, !isOpen && styles.menuClosed)}
style={{
...floatingStyles,
maxWidth: popoverMaxWidth,
minWidth: inputRef.current?.offsetWidth,
width: popoverWidth,
}}
{...getMenuProps({ ref: floatingRef })}
{...getMenuProps({
ref: floatingRef,
'aria-labelledby': ariaLabelledBy,
})}
>
{isOpen && (
<ul style={{ height: rowVirtualizer.getTotalSize() }} className={styles.menuUlContainer}>
@ -259,7 +274,10 @@ export const Combobox = ({
height: virtualRow.size,
transform: `translateY(${virtualRow.start}px)`,
}}
{...getItemProps({ item: items[virtualRow.index], index: virtualRow.index })}
{...getItemProps({
item: items[virtualRow.index],
index: virtualRow.index,
})}
>
<div className={styles.optionBody}>
<span className={styles.optionLabel}>{items[virtualRow.index].label}</span>
@ -278,7 +296,7 @@ export const Combobox = ({
};
const useDynamicWidth = (
items: Option[],
items: Array<ComboboxOption<string | number>>,
range: { startIndex: number; endIndex: number } | null,
setPopoverWidth: { (value: SetStateAction<number | undefined>): void }
) => {

View File

@ -4,12 +4,14 @@ import { GrafanaTheme2 } from '@grafana/data';
export const getComboboxStyles = (theme: GrafanaTheme2) => {
return {
menuClosed: css({
display: 'none',
}),
menu: css({
label: 'grafana-select-menu',
background: theme.components.dropdown.background,
boxShadow: theme.shadows.z3,
position: 'relative',
zIndex: 1,
zIndex: theme.zIndex.dropdown,
}),
menuHeight: css({
height: 400,

View File

@ -9,8 +9,8 @@ import { selectors } from '@grafana/e2e-selectors';
import { useStyles2 } from '../../../themes';
import { t, Trans } from '../../../utils/i18n';
import { Button } from '../../Button';
import { Combobox } from '../../Combobox/Combobox';
import { Field } from '../../Forms/Field';
import { Select } from '../../Select/Select';
import { Tab, TabContent, TabsBar } from '../../Tabs';
import { TimeZonePicker } from '../TimeZonePicker';
import { TimeZoneDescription } from '../TimeZonePicker/TimeZoneDescription';
@ -142,13 +142,12 @@ export const TimePickerFooter = (props: Props) => {
className={style.fiscalYearField}
label={t('time-picker.footer.fiscal-year-start', 'Fiscal year start month')}
>
<Select
value={fiscalYearStartMonth}
menuShouldPortal={false}
<Combobox
value={fiscalYearStartMonth ?? null}
options={monthOptions}
onChange={(value) => {
if (onChangeFiscalYearStartMonth) {
onChangeFiscalYearStartMonth(value.value ?? 0);
onChangeFiscalYearStartMonth(value?.value ?? 0);
}
}}
/>

View File

@ -1,9 +1,8 @@
import { useCallback } from 'react';
import { useCallback, useMemo } from 'react';
import { SelectableValue } from '@grafana/data';
import { selectors } from '@grafana/e2e-selectors';
import { Select } from '../Select/Select';
import { Combobox, ComboboxOption } from '../Combobox/Combobox';
export interface Props {
onChange: (weekStart: string) => void;
@ -16,7 +15,7 @@ export interface Props {
}
export type WeekStart = 'saturday' | 'sunday' | 'monday';
const weekStarts: Array<SelectableValue<WeekStart | ''>> = [
const weekStarts: ComboboxOption[] = [
{ value: '', label: 'Default' },
{ value: 'saturday', label: 'Saturday' },
{ value: 'sunday', label: 'Sunday' },
@ -39,21 +38,22 @@ export const WeekStartPicker = (props: Props) => {
const { onChange, width, autoFocus = false, onBlur, value, disabled = false, inputId } = props;
const onChangeWeekStart = useCallback(
(selectable: SelectableValue<string>) => {
if (selectable.value !== undefined) {
(selectable: ComboboxOption | null) => {
if (selectable && selectable.value !== undefined) {
onChange(selectable.value);
}
},
[onChange]
);
const selected = useMemo(() => weekStarts.find((item) => item.value === value)?.value ?? null, [value]);
return (
<Select
inputId={inputId}
value={weekStarts.find((item) => item.value === value)?.value}
<Combobox
id={inputId}
value={selected}
placeholder={selectors.components.WeekStartPicker.placeholder}
autoFocus={autoFocus}
openMenuOnFocus={true}
width={width}
options={weekStarts}
onChange={onChangeWeekStart}

View File

@ -1,4 +1,6 @@
import { SelectableValue, TimeOption } from '@grafana/data';
import { TimeOption } from '@grafana/data';
import { ComboboxOption } from '../Combobox/Combobox';
export const quickOptions: TimeOption[] = [
{ from: 'now-5m', to: 'now', display: 'Last 5 minutes' },
@ -39,7 +41,7 @@ export const quickOptions: TimeOption[] = [
{ from: 'now/fy', to: 'now/fy', display: 'This fiscal year' },
];
export const monthOptions: Array<SelectableValue<number>> = [
export const monthOptions: Array<ComboboxOption<number>> = [
{ label: 'January', value: 0 },
{ label: 'February', value: 1 },
{ label: 'March', value: 2 },

View File

@ -10,3 +10,4 @@
*/
export * from './utils/skeleton';
export * from './components/Combobox/Combobox';

View File

@ -1,11 +1,18 @@
import { render, screen, waitFor, within } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { comboboxTestSetup } from 'test/helpers/comboboxTestSetup';
import { getSelectParent, selectOptionInTest } from 'test/helpers/selectOptionInTest';
import { Preferences as UserPreferencesDTO } from '@grafana/schema/src/raw/preferences/x/preferences_types.gen';
import SharedPreferences from './SharedPreferences';
const selectComboboxOptionInTest = async (input: HTMLElement, optionOrOptions: string) => {
await userEvent.click(input);
const option = await screen.findByRole('option', { name: optionOrOptions });
await userEvent.click(option);
};
jest.mock('app/features/dashboard/api/dashboard_api', () => ({
getDashboardAPI: () => ({
getDashboardDTO: jest.fn().mockResolvedValue({
@ -114,6 +121,7 @@ describe('SharedPreferences', () => {
configurable: true,
value: { reload: mockReload },
});
comboboxTestSetup();
});
afterAll(() => {
@ -129,9 +137,9 @@ describe('SharedPreferences', () => {
await waitFor(() => expect(mockPrefsLoad).toHaveBeenCalled());
});
it('renders the theme preference', () => {
const themeSelect = getSelectParent(screen.getByLabelText('Interface theme'));
expect(themeSelect).toHaveTextContent('Light');
it('renders the theme preference', async () => {
const themeSelect = await screen.findByRole('combobox', { name: 'Interface theme' });
expect(themeSelect).toHaveValue('Light');
});
it('renders the home dashboard preference', async () => {
@ -147,20 +155,20 @@ describe('SharedPreferences', () => {
});
it('renders the week start preference', async () => {
const weekSelect = getSelectParent(screen.getByLabelText('Week start'));
expect(weekSelect).toHaveTextContent('Monday');
const weekSelect = await screen.findByRole('combobox', { name: 'Week start' });
expect(weekSelect).toHaveValue('Monday');
});
it('renders the language preference', async () => {
const weekSelect = getSelectParent(screen.getByLabelText(/language/i));
expect(weekSelect).toHaveTextContent('Default');
it('renders the default language preference', async () => {
const langSelect = await screen.findByRole('combobox', { name: /language/i });
expect(langSelect).toHaveValue('Default');
});
it('saves the users new preferences', async () => {
await selectOptionInTest(screen.getByLabelText('Interface theme'), 'Dark');
await selectComboboxOptionInTest(await screen.findByRole('combobox', { name: 'Interface theme' }), 'Dark');
await selectOptionInTest(screen.getByLabelText('Timezone'), 'Australia/Sydney');
await selectOptionInTest(screen.getByLabelText('Week start'), 'Saturday');
await selectOptionInTest(screen.getByLabelText(/language/i), 'Français');
await selectComboboxOptionInTest(await screen.findByRole('combobox', { name: 'Week start' }), 'Saturday');
await selectComboboxOptionInTest(await screen.findByRole('combobox', { name: /language/i }), 'Français');
await userEvent.click(screen.getByText('Save'));
@ -177,7 +185,7 @@ describe('SharedPreferences', () => {
});
it('saves the users default preferences', async () => {
await selectOptionInTest(screen.getByLabelText('Interface theme'), 'Default');
await selectComboboxOptionInTest(await screen.findByRole('combobox', { name: 'Interface theme' }), 'Default');
// there's no default option in this dropdown - there's a clear selection button
// get the parent container, and find the "select-clear-value" button
@ -185,8 +193,10 @@ describe('SharedPreferences', () => {
await userEvent.click(within(dashboardSelect).getByRole('button', { name: 'select-clear-value' }));
await selectOptionInTest(screen.getByLabelText('Timezone'), 'Default');
await selectOptionInTest(screen.getByLabelText('Week start'), 'Default');
await selectOptionInTest(screen.getByLabelText(/language/i), 'Default');
await selectComboboxOptionInTest(await screen.findByRole('combobox', { name: 'Week start' }), 'Default');
await selectComboboxOptionInTest(screen.getByRole('combobox', { name: /language/i }), 'Default');
await userEvent.click(screen.getByText('Save'));
expect(mockPrefsUpdate).toHaveBeenCalledWith(defaultPreferences);

View File

@ -2,7 +2,7 @@ import { css } from '@emotion/css';
import { PureComponent } from 'react';
import * as React from 'react';
import { FeatureState, SelectableValue, getBuiltInThemes, ThemeRegistryItem } from '@grafana/data';
import { FeatureState, getBuiltInThemes, ThemeRegistryItem } from '@grafana/data';
import { selectors } from '@grafana/e2e-selectors';
import { config, reportInteraction } from '@grafana/runtime';
import { Preferences as UserPreferencesDTO } from '@grafana/schema/src/raw/preferences/x/preferences_types.gen';
@ -11,12 +11,12 @@ import {
Field,
FieldSet,
Label,
Select,
stylesFactory,
TimeZonePicker,
WeekStartPicker,
FeatureBadge,
} from '@grafana/ui';
import { Combobox, ComboboxOption } from '@grafana/ui/src/unstable';
import { DashboardPicker } from 'app/core/components/Select/DashboardPicker';
import { t, Trans } from 'app/core/internationalization';
import { LANGUAGES, PSEUDO_LOCALE } from 'app/core/internationalization/constants';
@ -32,7 +32,7 @@ export interface Props {
export type State = UserPreferencesDTO;
function getLanguageOptions(): Array<SelectableValue<string>> {
function getLanguageOptions(): ComboboxOption[] {
const languageOptions = LANGUAGES.map((v) => ({
value: v.code,
label: v.name,
@ -61,7 +61,7 @@ function getLanguageOptions(): Array<SelectableValue<string>> {
export class SharedPreferences extends PureComponent<Props, State> {
service: PreferencesService;
themeOptions: SelectableValue[];
themeOptions: ComboboxOption[];
constructor(props: Props) {
super(props);
@ -110,7 +110,10 @@ export class SharedPreferences extends PureComponent<Props, State> {
}
};
onThemeChanged = (value: SelectableValue<string>) => {
onThemeChanged = (value: ComboboxOption<string> | null) => {
if (!value) {
return;
}
this.setState({ theme: value.value });
if (value.value) {
@ -153,11 +156,11 @@ export class SharedPreferences extends PureComponent<Props, State> {
<form onSubmit={this.onSubmitForm} className={styles.form}>
<FieldSet label={<Trans i18nKey="shared-preferences.title">Preferences</Trans>} disabled={disabled}>
<Field label={t('shared-preferences.fields.theme-label', 'Interface theme')}>
<Select
<Combobox
options={this.themeOptions}
value={currentThemeOption}
value={currentThemeOption.value}
onChange={this.onThemeChanged}
inputId="shared-preferences-theme-select"
id="shared-preferences-theme-select"
/>
</Field>
@ -215,12 +218,12 @@ export class SharedPreferences extends PureComponent<Props, State> {
}
data-testid="User preferences language drop down"
>
<Select
value={languages.find((lang) => lang.value === language)}
onChange={(lang: SelectableValue<string>) => this.onLanguageChanged(lang.value ?? '')}
<Combobox
value={languages.find((lang) => lang.value === language)?.value || ''}
onChange={(lang: ComboboxOption | null) => this.onLanguageChanged(lang?.value ?? '')}
options={languages}
placeholder={t('shared-preferences.fields.locale-placeholder', 'Choose language')}
inputId="locale-select"
id="locale-select"
/>
</Field>
</FieldSet>

View File

@ -0,0 +1,17 @@
/**
* Needed for Combobox virtual list. The numbers are arbitrary and just need to be consistent.
*/
export const comboboxTestSetup = () => {
const mockGetBoundingClientRect = jest.fn(() => ({
width: 120,
height: 120,
top: 0,
left: 0,
bottom: 0,
right: 0,
}));
Object.defineProperty(Element.prototype, 'getBoundingClientRect', {
value: mockGetBoundingClientRect,
});
};