From 096105fed68c91fa77c94bbbcca2bc02b4bb968d Mon Sep 17 00:00:00 2001 From: Tobias Skarhed <1438972+tskarhed@users.noreply.github.com> Date: Tue, 10 Sep 2024 16:28:00 +0200 Subject: [PATCH] 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 --- .betterer.results | 4 + .../components/Combobox/Combobox.story.tsx | 8 +- .../src/components/Combobox/Combobox.test.tsx | 51 +++++++++++-- .../src/components/Combobox/Combobox.tsx | 76 ++++++++++++------- .../components/Combobox/getComboboxStyles.ts | 6 +- .../TimeRangePicker/TimePickerFooter.tsx | 9 +-- .../DateTimePickers/WeekStartPicker.tsx | 20 ++--- .../src/components/DateTimePickers/options.ts | 6 +- packages/grafana-ui/src/unstable.ts | 1 + .../SharedPreferences.test.tsx | 38 ++++++---- .../SharedPreferences/SharedPreferences.tsx | 27 ++++--- public/test/helpers/comboboxTestSetup.ts | 17 +++++ 12 files changed, 180 insertions(+), 83 deletions(-) create mode 100644 public/test/helpers/comboboxTestSetup.ts diff --git a/.betterer.results b/.betterer.results index 29f105c7116..1d379783c7f 100644 --- a/.betterer.results +++ b/.betterer.results @@ -595,6 +595,10 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], [0, 0, 0, "No untranslated strings. Wrap text with ", "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 ", "0"] ], diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx index 20889e6ed1c..d5846b3025a 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx @@ -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; export const Basic: Story = {}; -async function generateOptions(amount: number): Promise { +async function generateOptions(amount: number): Promise { 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 { } const ManyOptionsStory: StoryFn = ({ numberOfOptions, ...args }) => { - const [value, setValue] = useState(null); - const [options, setOptions] = useState([]); + const [value, setValue] = useState(null); + const [options, setOptions] = useState([]); const [isLoading, setIsLoading] = useState(true); useEffect(() => { diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx index 2263618d601..46b7d48a404 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx @@ -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(); + 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(); + 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(); + }); + }); }); diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.tsx index 3fe19ab9ac7..cb8a5f7bbba 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.tsx @@ -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 = { label: string; - value: Value; + value: T; description?: string; }; -interface ComboboxProps +interface ComboboxProps extends Omit { - onChange: (val: Option | null) => void; - value: Value | null; - options: Option[]; isClearable?: boolean; createCustomValue?: boolean; + options: Array>; + onChange: (option: ComboboxOption | null) => void; + value: T | null; } -function itemToString(item: Option | null) { - return item?.label ?? item?.value?.toString() ?? ''; +function itemToString(item: ComboboxOption | null) { + return item?.label ?? item?.value.toString() ?? ''; } -function itemFilter(inputValue: string) { +function itemFilter(inputValue: string) { const lowerCasedInputValue = inputValue.toLowerCase(); - return (item: Option) => { + return (item: ComboboxOption) => { 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 = ({ options, onChange, value, isClearable = false, createCustomValue = false, id, + 'aria-labelledby': ariaLabelledBy, ...restProps -}: ComboboxProps) => { +}: ComboboxProps) => { 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(null); const floatingRef = useRef(null); + const menuId = `downshift-${useId().replace(/:/g, '--')}-menu`; + const labelId = `downshift-${useId().replace(/:/g, '--')}-label`; + const styles = useStyles2(getComboboxStyles); const [popoverMaxWidth, setPopoverMaxWidth] = useState(undefined); const [popoverWidth, setPopoverWidth] = useState(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 = { + 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 })} />
{isOpen && (
    @@ -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, + })} >
    {items[virtualRow.index].label} @@ -278,7 +296,7 @@ export const Combobox = ({ }; const useDynamicWidth = ( - items: Option[], + items: Array>, range: { startIndex: number; endIndex: number } | null, setPopoverWidth: { (value: SetStateAction): void } ) => { diff --git a/packages/grafana-ui/src/components/Combobox/getComboboxStyles.ts b/packages/grafana-ui/src/components/Combobox/getComboboxStyles.ts index 06fa657821c..ae5c94bc409 100644 --- a/packages/grafana-ui/src/components/Combobox/getComboboxStyles.ts +++ b/packages/grafana-ui/src/components/Combobox/getComboboxStyles.ts @@ -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, diff --git a/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimePickerFooter.tsx b/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimePickerFooter.tsx index 28b328ed5c3..7ac73a45cb4 100644 --- a/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimePickerFooter.tsx +++ b/packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimePickerFooter.tsx @@ -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')} > - item.value === value)?.value} + > = [ +export const monthOptions: Array> = [ { label: 'January', value: 0 }, { label: 'February', value: 1 }, { label: 'March', value: 2 }, diff --git a/packages/grafana-ui/src/unstable.ts b/packages/grafana-ui/src/unstable.ts index 02938ddf2d2..7495a57315e 100644 --- a/packages/grafana-ui/src/unstable.ts +++ b/packages/grafana-ui/src/unstable.ts @@ -10,3 +10,4 @@ */ export * from './utils/skeleton'; +export * from './components/Combobox/Combobox'; diff --git a/public/app/core/components/SharedPreferences/SharedPreferences.test.tsx b/public/app/core/components/SharedPreferences/SharedPreferences.test.tsx index 5b61d50da72..84346d2ed52 100644 --- a/public/app/core/components/SharedPreferences/SharedPreferences.test.tsx +++ b/public/app/core/components/SharedPreferences/SharedPreferences.test.tsx @@ -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); diff --git a/public/app/core/components/SharedPreferences/SharedPreferences.tsx b/public/app/core/components/SharedPreferences/SharedPreferences.tsx index 47e063ac363..8bde5bd7b4e 100644 --- a/public/app/core/components/SharedPreferences/SharedPreferences.tsx +++ b/public/app/core/components/SharedPreferences/SharedPreferences.tsx @@ -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> { +function getLanguageOptions(): ComboboxOption[] { const languageOptions = LANGUAGES.map((v) => ({ value: v.code, label: v.name, @@ -61,7 +61,7 @@ function getLanguageOptions(): Array> { export class SharedPreferences extends PureComponent { service: PreferencesService; - themeOptions: SelectableValue[]; + themeOptions: ComboboxOption[]; constructor(props: Props) { super(props); @@ -110,7 +110,10 @@ export class SharedPreferences extends PureComponent { } }; - onThemeChanged = (value: SelectableValue) => { + onThemeChanged = (value: ComboboxOption | null) => { + if (!value) { + return; + } this.setState({ theme: value.value }); if (value.value) { @@ -153,11 +156,11 @@ export class SharedPreferences extends PureComponent {
    Preferences} disabled={disabled}> - lang.value === language)} - onChange={(lang: SelectableValue) => this.onLanguageChanged(lang.value ?? '')} + 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" />
    diff --git a/public/test/helpers/comboboxTestSetup.ts b/public/test/helpers/comboboxTestSetup.ts new file mode 100644 index 00000000000..f36d9b56cb8 --- /dev/null +++ b/public/test/helpers/comboboxTestSetup.ts @@ -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, + }); +};