From ed31457c004b0df528ffb984bb37a9fbc8152220 Mon Sep 17 00:00:00 2001 From: Tobias Skarhed <1438972+tskarhed@users.noreply.github.com> Date: Tue, 19 Nov 2024 11:52:52 +0100 Subject: [PATCH] Combobox: Support undefined, null value and improve typing (#96523) * Support undefined value * Check truthiness of value instead * check falsy * Conditional typing for clearing value * Less restrictive default typing * simplify props * Add tests for autosizing * Write failing test case * Add list of falsy values * Check if nullish * Check nullish in itemToString * Nvm, it doesn't matter here * Add support for autoFocus * Pick from InputProps * Move docstring * Solve type issues in Storybook * Fix failing story --- .../components/Combobox/Combobox.story.tsx | 173 ++---------------- .../src/components/Combobox/Combobox.test.tsx | 79 +++++++- .../src/components/Combobox/Combobox.tsx | 66 +++++-- .../SharedPreferences/SharedPreferences.tsx | 5 +- 4 files changed, 148 insertions(+), 175 deletions(-) diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx index 1b63c36bec1..7bb767e9dbd 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx @@ -4,16 +4,16 @@ import React, { ComponentProps, useCallback, useEffect, useState } from 'react'; import { SelectableValue } from '@grafana/data'; -import { useTheme2 } from '../../themes/ThemeContext'; import { Alert } from '../Alert/Alert'; -import { Divider } from '../Divider/Divider'; import { Field } from '../Forms/Field'; -import { AsyncSelect, Select } from '../Select/Select'; +import { AsyncSelect } from '../Select/Select'; import { Combobox, ComboboxOption } from './Combobox'; import mdx from './Combobox.mdx'; -type PropsAndCustomArgs = ComponentProps & { numberOfOptions: number }; +type PropsAndCustomArgs = ComponentProps> & { + numberOfOptions: number; +}; const meta: Meta = { title: 'Forms/Combobox', @@ -27,6 +27,7 @@ const meta: Meta = { loading: undefined, invalid: undefined, width: undefined, + isClearable: false, placeholder: 'Select an option...', options: [ { label: 'Apple', value: 'apple' }, @@ -45,12 +46,6 @@ const meta: Meta = { { label: 'Honeydew', value: 'honeydew' }, { label: 'Iceberg Lettuce', value: 'iceberg-lettuce' }, { label: 'Jackfruit', value: 'jackfruit' }, - { label: '1', value: 1 }, - { label: '2', value: 2 }, - { label: '3', value: 3 }, - { label: '4', value: 4 }, - { label: '5', value: 5 }, - { label: '6', value: 6 }, ], value: 'banana', }, @@ -59,16 +54,16 @@ const meta: Meta = { decorators: [InDevDecorator], }; -const BasicWithState: StoryFn = (args) => { - const [value, setValue] = useState(args.value); - +const BasicWithState: StoryFn = (args) => { + const [value, setValue] = useState(); return ( { + onChange={(val: ComboboxOption | null) => { + // TODO: Figure out how to update value on args setValue(val?.value || null); action('onChange')(val); }} @@ -88,7 +83,7 @@ async function generateOptions(amount: number): Promise { })); } -const ManyOptionsStory: StoryFn = ({ numberOfOptions, ...args }) => { +const ManyOptionsStory: StoryFn> = ({ numberOfOptions, ...args }) => { const [value, setValue] = useState(null); const [options, setOptions] = useState([]); const [isLoading, setIsLoading] = useState(true); @@ -103,13 +98,14 @@ const ManyOptionsStory: StoryFn = ({ numberOfOptions, ...arg }, 1000); }, [numberOfOptions]); + const { onChange, ...rest } = args; return ( { + onChange={(opt: ComboboxOption | null) => { setValue(opt?.value || null); action('onChange')(opt); }} @@ -117,132 +113,6 @@ const ManyOptionsStory: StoryFn = ({ numberOfOptions, ...arg ); }; -const SelectComparisonStory: StoryFn = (args) => { - const [comboboxValue, setComboboxValue] = useState(args.value); - const theme = useTheme2(); - - if (typeof args.options === 'function') { - throw new Error('This story does not support async options'); - } - - return ( -
- - { - setComboboxValue(val?.value || null); - action('onChange')(val); - }} - /> - - - - { - setComboboxValue(val?.value || null); - action('onChange')(val); - }} - /> - - - - - - { - setComboboxValue(val?.value || null); - action('onChange')(val); - }} - /> - - - - { - setComboboxValue(val?.value || null); - action('onChange')(val); - }} - /> - -
- ); -}; - export const AutoSize: StoryObj = { args: { width: 'auto', @@ -294,6 +164,8 @@ const AsyncStory: StoryFn = (args) => { } }, []); + const { onChange, ...rest } = args; + return ( <> = (args) => { description="This tests when options have both a label and a value. Consumers are required to pass in a full ComboboxOption as a value with a label" > { + onChange={(val: ComboboxOption | null) => { action('onChange')(val); setSelectedOption(val); }} @@ -324,7 +196,7 @@ const AsyncStory: StoryFn = (args) => { placeholder="Select an option" options={loadOptionsOnlyValues} value={selectedOption?.value ?? null} - onChange={(val) => { + onChange={(val: ComboboxOption | null) => { action('onChange')(val); setSelectedOption(val); }} @@ -366,7 +238,7 @@ const AsyncStory: StoryFn = (args) => { placeholder="Select an option" options={loadOptionsWithErrors} value={selectedOption} - onChange={(val) => { + onChange={(val: ComboboxOption | null) => { action('onChange')(val); setSelectedOption(val); }} @@ -426,13 +298,6 @@ export const PositioningTest: StoryObj = { render: PositioningTestStory, }; -export const ComparisonToSelect: StoryObj = { - args: { - numberOfOptions: 100, - }, - render: SelectComparisonStory, -}; - export default meta; function InDevDecorator(Story: React.ElementType) { diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx index d45c2cc2402..ab9e63fbb3b 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx @@ -1,5 +1,6 @@ import { act, render, screen, fireEvent } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; +import React from 'react'; import { Combobox, ComboboxOption } from './Combobox'; @@ -49,7 +50,7 @@ describe('Combobox', () => { expect(onChangeHandler).toHaveBeenCalledWith(options[0]); }); - it("shows the placeholder with the menu open when there's no value", async () => { + it('shows the placeholder with the menu open when value is null', async () => { render(); const input = screen.getByRole('combobox'); @@ -58,6 +59,15 @@ describe('Combobox', () => { expect(input).toHaveAttribute('placeholder', 'Select an option'); }); + it('shows the placeholder with the menu open when value is undefined', async () => { + render(); + + const input = screen.getByRole('combobox'); + await userEvent.click(input); + + expect(input).toHaveAttribute('placeholder', 'Select an option'); + }); + it('selects value by clicking that needs scrolling', async () => { render(); @@ -106,6 +116,73 @@ describe('Combobox', () => { expect(screen.queryByDisplayValue('Option 2')).not.toBeInTheDocument(); }); + it.each(['very valid value', '', 0])('should handle an option with %p as a value', async (val) => { + const options = [ + { label: 'Second option', value: '2' }, + { label: 'Default', value: val }, + ]; + + const ControlledCombobox = () => { + const [value, setValue] = React.useState(null); + + return ( + { + setValue(opt.value); + }} + /> + ); + }; + + render(); + + const input = screen.getByRole('combobox'); + await userEvent.click(input); + await userEvent.click(screen.getByRole('option', { name: 'Default' })); + expect(screen.queryByDisplayValue('Default')).toBeInTheDocument(); + + await userEvent.click(input); + + expect(screen.getByRole('option', { name: 'Default' })).toHaveAttribute('aria-selected', 'true'); + }); + + describe('size support', () => { + it('should require minWidth to be set with auto width', () => { + // @ts-expect-error + render(); + }); + + it('should change width when typing things with auto width', async () => { + render(); + + const input = screen.getByRole('combobox'); + const inputWrapper = screen.getByTestId('input-wrapper'); + const initialWidth = getComputedStyle(inputWrapper).width; + + fireEvent.change(input, { target: { value: 'very very long value' } }); + + const newWidth = getComputedStyle(inputWrapper).width; + + expect(initialWidth).not.toBe(newWidth); + }); + + it('should not change width when typing things with fixed width', async () => { + render(); + const input = screen.getByRole('combobox'); + + const inputWrapper = screen.getByTestId('input-wrapper'); + const initialWidth = getComputedStyle(inputWrapper).width; + + fireEvent.change(input, { target: { value: 'very very long value' } }); + + const newWidth = getComputedStyle(inputWrapper).width; + + expect(initialWidth).toBe(newWidth); + }); + }); + describe('with a value already selected', () => { it('shows an empty text input when opening the menu', async () => { const selectedValue = options[0].value; diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.tsx index 3b70f8e3f78..c3940a7fed1 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.tsx @@ -28,7 +28,10 @@ export type ComboboxOption = { // TODO: It would be great if ComboboxOption["label"] was more generic so that if consumers do pass it in (for async), // then the onChange handler emits ComboboxOption with the label as non-undefined. interface ComboboxBaseProps - extends Omit { + extends Pick< + InputProps, + 'placeholder' | 'autoFocus' | 'id' | 'aria-labelledby' | 'disabled' | 'loading' | 'invalid' + > { /** * An `X` appears in the UI, which clears the input and sets the value to `null`. Do not use if you have no `null` case. */ @@ -38,20 +41,31 @@ interface ComboboxBaseProps */ createCustomValue?: boolean; options: Array> | ((inputValue: string) => Promise>>); - onChange: (option: ComboboxOption | null) => void; + onChange: (option: ComboboxOption) => void; /** * Most consumers should pass value in as a scalar string | number. However, sometimes with Async because we don't * have the full options loaded to match the value to, consumers may also pass in an Option with a label to display. */ - value: T | ComboboxOption | null; + value?: T | ComboboxOption | null; /** * Defaults to 100%. Number is a multiple of 8px. 'auto' will size the input to the content. * */ width?: number | 'auto'; + onBlur?: () => void; } const RECOMMENDED_ITEMS_AMOUNT = 100_000; +type ClearableConditionals = + | { + isClearable: true; + /** + * The onChange handler is called with `null` when clearing the Combobox. + */ + onChange: (option: ComboboxOption | null) => void; + } + | { isClearable?: false; onChange: (option: ComboboxOption) => void }; + type AutoSizeConditionals = | { width: 'auto'; @@ -70,13 +84,16 @@ type AutoSizeConditionals = maxWidth?: never; }; -type ComboboxProps = ComboboxBaseProps & AutoSizeConditionals; +type ComboboxProps = ComboboxBaseProps & AutoSizeConditionals & ClearableConditionals; -function itemToString(item: ComboboxOption | null) { - if (item?.label?.includes('Custom value: ')) { - return item?.value.toString(); +function itemToString(item?: ComboboxOption | null) { + if (!item) { + return ''; } - return item?.label ?? item?.value.toString() ?? ''; + if (item.label?.includes('Custom value: ')) { + return item.value.toString(); + } + return item.label ?? item.value.toString(); } function itemFilter(inputValue: string) { @@ -85,8 +102,8 @@ function itemFilter(inputValue: string) { return (item: ComboboxOption) => { return ( !inputValue || - item?.label?.toLowerCase().includes(lowerCasedInputValue) || - item?.value?.toString().toLowerCase().includes(lowerCasedInputValue) + item.label?.toLowerCase().includes(lowerCasedInputValue) || + item.value?.toString().toLowerCase().includes(lowerCasedInputValue) ); }; } @@ -109,8 +126,14 @@ export const Combobox = (props: ComboboxProps) => createCustomValue = false, id, width, + minWidth, + maxWidth, 'aria-labelledby': ariaLabelledBy, - ...restProps + autoFocus, + onBlur, + disabled, + loading, + invalid, } = props; // Value can be an actual scalar Value (string or number), or an Option (value + label), so @@ -158,7 +181,7 @@ export const Combobox = (props: ComboboxProps) => return null; } - if (value === null) { + if (valueProp === undefined || valueProp === null) { return null; } @@ -168,9 +191,13 @@ export const Combobox = (props: ComboboxProps) => } return index; - }, [options, value, isAsync]); + }, [valueProp, options, value, isAsync]); const selectedItem = useMemo(() => { + if (valueProp === undefined || valueProp === null) { + return null; + } + if (selectedItemIndex !== null && !isAsync) { return options[selectedItemIndex]; } @@ -329,7 +356,9 @@ export const Combobox = (props: ComboboxProps) => const { inputRef, floatingRef, floatStyles, scrollRef } = useComboboxFloat(items, rowVirtualizer.range, isOpen); - const InputComponent = width === 'auto' ? AutoSizeInput : Input; + const isAutoSize = width === 'auto'; + + const InputComponent = isAutoSize ? AutoSizeInput : Input; const suffixIcon = asyncLoading ? 'spinner' @@ -343,7 +372,13 @@ export const Combobox = (props: ComboboxProps) => return (
@@ -368,7 +403,6 @@ export const Combobox = (props: ComboboxProps) => } - {...restProps} {...getInputProps({ ref: inputRef, /* Empty onCall to avoid TS error diff --git a/public/app/core/components/SharedPreferences/SharedPreferences.tsx b/public/app/core/components/SharedPreferences/SharedPreferences.tsx index dabf652a4da..cd6475c45ac 100644 --- a/public/app/core/components/SharedPreferences/SharedPreferences.tsx +++ b/public/app/core/components/SharedPreferences/SharedPreferences.tsx @@ -111,10 +111,7 @@ export class SharedPreferences extends PureComponent { } }; - onThemeChanged = (value: ComboboxOption | null) => { - if (!value) { - return; - } + onThemeChanged = (value: ComboboxOption) => { this.setState({ theme: value.value }); if (value.value) {