From e8241636b8a9412ad3ea29d5ab7f1961c886457d Mon Sep 17 00:00:00 2001 From: Josh Hunt Date: Mon, 11 Nov 2024 14:35:37 +0000 Subject: [PATCH] Combobox: Improve async UX (#96054) * Refactor basic usage with stateReducer This is a combination of 3 commits. This is the 1st commit message: more wip This is the commit message #2: even more wip This is the commit message #3: got basic usage working well with stateReducer remove unrelated change todo tests * fix behaviour for async * clean up dev stuff * story * Fix options being cleared for non-async combobox * Fill out tests! * put story back * clean up metriccombobox test * show selected value as placeholder while menu is open * properly fallback placeholder to the prop --- .../components/MetricCombobox.test.tsx | 4 +- .../components/Combobox/Combobox.story.tsx | 22 ++- .../src/components/Combobox/Combobox.test.tsx | 115 ++++++++++++ .../src/components/Combobox/Combobox.tsx | 169 ++++++++++++------ 4 files changed, 251 insertions(+), 59 deletions(-) diff --git a/packages/grafana-prometheus/src/querybuilder/components/MetricCombobox.test.tsx b/packages/grafana-prometheus/src/querybuilder/components/MetricCombobox.test.tsx index 4397d4104de..d23fda227f7 100644 --- a/packages/grafana-prometheus/src/querybuilder/components/MetricCombobox.test.tsx +++ b/packages/grafana-prometheus/src/querybuilder/components/MetricCombobox.test.tsx @@ -104,14 +104,12 @@ describe('MetricCombobox', () => { const combobox = screen.getByPlaceholderText('Select metric'); await userEvent.click(combobox); - await userEvent.type(combobox, 'unique'); - expect(jest.mocked(mockDatasource.metricFindQuery)).toHaveBeenCalled(); const item = await screen.findByRole('option', { name: 'unique_metric' }); expect(item).toBeInTheDocument(); - const negativeItem = await screen.queryByRole('option', { name: 'random_metric' }); + const negativeItem = screen.queryByRole('option', { name: 'random_metric' }); expect(negativeItem).not.toBeInTheDocument(); }); diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx index b1c528cc2a7..967b7537b8d 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.story.tsx @@ -8,7 +8,7 @@ import { useTheme2 } from '../../themes/ThemeContext'; import { Alert } from '../Alert/Alert'; import { Divider } from '../Divider/Divider'; import { Field } from '../Forms/Field'; -import { Select, AsyncSelect } from '../Select/Select'; +import { AsyncSelect, Select } from '../Select/Select'; import { Combobox, ComboboxOption } from './Combobox'; @@ -55,6 +55,7 @@ const meta: Meta = { const BasicWithState: StoryFn = (args) => { const [value, setValue] = useState(args.value); + return ( = { }, }; +const loadOptionsAction = action('loadOptions called'); const AsyncStory: StoryFn = (args) => { // Combobox const [selectedOption, setSelectedOption] = useState | null>(null); @@ -268,7 +270,7 @@ const AsyncStory: StoryFn = (args) => { // This simulates a kind of search API call const loadOptionsWithLabels = useCallback((inputValue: string) => { - console.info(`Load options called with value '${inputValue}' `); + loadOptionsAction(inputValue); return fakeSearchAPI(`http://example.com/search?query=${inputValue}`); }, []); @@ -326,7 +328,6 @@ const AsyncStory: StoryFn = (args) => { = (args) => { }} /> + = (args) => { }} /> + + + { + action('onChange')(val); + setSelectedOption(val); + }} + /> + ); }; diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx index 87a84c7e3d3..1c3ac5766da 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx @@ -28,6 +28,10 @@ describe('Combobox', () => { }); }); + afterEach(() => { + onChangeHandler.mockReset(); + }); + it('renders without error', () => { render(); expect(screen.getByRole('combobox')).toBeInTheDocument(); @@ -45,6 +49,15 @@ describe('Combobox', () => { expect(onChangeHandler).toHaveBeenCalledWith(options[0]); }); + it("shows the placeholder with the menu open when there's no value", 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(); @@ -93,6 +106,54 @@ describe('Combobox', () => { expect(screen.queryByDisplayValue('Option 2')).not.toBeInTheDocument(); }); + describe('with a value already selected', () => { + it('shows an empty text input when opening the menu', async () => { + const selectedValue = options[0].value; + render(); + expect(screen.getByRole('combobox')).toBeInTheDocument(); + + const input = screen.getByRole('combobox'); + await userEvent.click(input); + + expect(input).toHaveValue(''); + }); + + it('shows all options unfiltered when opening the menu', async () => { + const selectedValue = options[0].value; + render(); + expect(screen.getByRole('combobox')).toBeInTheDocument(); + + const input = screen.getByRole('combobox'); + await userEvent.click(input); + + const optionsEls = await screen.findAllByRole('option'); + expect(optionsEls).toHaveLength(options.length); + }); + + it('shows the current selected value as the placeholder of the input', async () => { + const selectedValue = options[0].value; + render(); + expect(screen.getByRole('combobox')).toBeInTheDocument(); + + const input = screen.getByRole('combobox'); + await userEvent.click(input); + + expect(input).toHaveAttribute('placeholder', options[0].label); + }); + + it('exiting the menu without selecting an item restores the value to the text input', async () => { + const selectedValue = options[0].value; + render(); + + const input = screen.getByRole('combobox'); + await userEvent.type(input, 'Option 3'); + await userEvent.keyboard('{Esc}'); + + expect(onChangeHandler).not.toHaveBeenCalled(); + expect(input).toHaveValue('Option 1'); + }); + }); + describe('create custom value', () => { it('should allow creating a custom value', async () => { const onChangeHandler = jest.fn(); @@ -156,6 +217,8 @@ describe('Combobox', () => { const input = screen.getByRole('combobox'); await user.click(input); + await act(async () => jest.advanceTimersByTime(200)); + expect(asyncOptions).toHaveBeenCalled(); }); @@ -294,5 +357,57 @@ describe('Combobox', () => { expect(emptyMessage).toBeInTheDocument(); }); + + describe('with a value already selected', () => { + const selectedValue = { value: '1', label: 'Option 1' }; + + it('shows an empty text input when opening the menu', async () => { + const asyncOptions = jest.fn(() => Promise.resolve(simpleAsyncOptions)); + render(); + + const input = screen.getByRole('combobox'); + await user.click(input); + + // Flush out async on open changes + await act(async () => Promise.resolve()); + + expect(input).toHaveValue(''); + }); + + it('shows all options unfiltered when opening the menu', async () => { + const asyncOptions = jest.fn(() => Promise.resolve(simpleAsyncOptions)); + render(); + + const input = screen.getByRole('combobox'); + await user.click(input); + + // Flush out async on open changes + await act(async () => Promise.resolve()); + + const optionsEls = await screen.findAllByRole('option'); + expect(optionsEls).toHaveLength(simpleAsyncOptions.length); + }); + + it('exiting the menu without selecting an item restores the value to the text input', async () => { + const asyncOptions = jest.fn(async () => { + return new Promise((resolve) => setTimeout(() => resolve([{ value: 'first' }]), 2000)); + }); + + render(); + + const input = screen.getByRole('combobox'); + await user.click(input); + + await act(async () => { + await user.type(input, 'Opt'); + jest.advanceTimersByTime(500); // Custom value while typing + }); + + await user.keyboard('{Esc}'); + + expect(onChangeHandler).not.toHaveBeenCalled(); + expect(input).toHaveValue('Option 1'); + }); + }); }); }); diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.tsx index ad9a8da6a60..23b9402e226 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.tsx @@ -72,6 +72,7 @@ function itemFilter(inputValue: string) { }; } +const noop = () => {}; const asyncNoop = () => Promise.resolve([]); /** @@ -79,17 +80,20 @@ const asyncNoop = () => Promise.resolve([]); * * @alpha */ -export const Combobox = ({ - options, - onChange, - value: valueProp, - isClearable = false, - createCustomValue = false, - id, - width, - 'aria-labelledby': ariaLabelledBy, - ...restProps -}: ComboboxProps) => { +export const Combobox = (props: ComboboxProps) => { + const { + options, + onChange, + value: valueProp, + placeholder: placeholderProp, + isClearable = false, + createCustomValue = false, + id, + width, + 'aria-labelledby': ariaLabelledBy, + ...restProps + } = props; + // Value can be an actual scalar Value (string or number), or an Option (value + label), so // get a consistent Value from it const value = typeof valueProp === 'object' ? valueProp?.value : valueProp; @@ -99,7 +103,31 @@ export const Combobox = ({ const [asyncLoading, setAsyncLoading] = useState(false); const [asyncError, setAsyncError] = useState(false); - const [items, setItems] = useState(isAsync ? [] : options); + // A custom setter to always prepend the custom value at the beginning, if needed + const [items, baseSetItems] = useState(isAsync ? [] : options); + const setItems = useCallback( + (items: Array>, inputValue: string | undefined) => { + let itemsToSet = items; + + if (inputValue && createCustomValue) { + const optionMatchingInput = items.find((opt) => opt.label === inputValue || opt.value === inputValue); + + if (!optionMatchingInput) { + const customValueOption = { + // 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'), + }; + + itemsToSet = items.slice(0); + itemsToSet.unshift(customValueOption); + } + } + + baseSetItems(itemsToSet); + }, + [createCustomValue] + ); const selectedItemIndex = useMemo(() => { if (isAsync) { @@ -142,10 +170,10 @@ export const Combobox = ({ const debounceAsync = useMemo( () => - debounce((inputValue: string, customValueOption: ComboboxOption | null) => { + debounce((inputValue: string) => { loadOptions(inputValue) .then((opts) => { - setItems(customValueOption ? [customValueOption, ...opts] : opts); + setItems(opts, inputValue); setAsyncLoading(false); setAsyncError(false); }) @@ -156,16 +184,17 @@ export const Combobox = ({ } }); }, 200), - [loadOptions] + [loadOptions, setItems] ); const { + isOpen, + highlightedIndex, + getInputProps, getMenuProps, getItemProps, - isOpen, - highlightedIndex, - setInputValue, + openMenu, closeMenu, selectItem, @@ -176,51 +205,53 @@ export const Combobox = ({ items, itemToString, selectedItem, + + // Don't change downshift state in the onBlahChange handlers. Instead, use the stateReducer to make changes. + // Downshift calls change handlers on the render after so you can get sync/flickering issues if you change its state + // in them. + // Instead, stateReducer is called in the same tick as state changes, before that state is committed and rendered. + onSelectedItemChange: ({ selectedItem }) => { onChange(selectedItem); }, + defaultHighlightedIndex: selectedItemIndex ?? 0, scrollIntoView: () => {}, - onInputValueChange: ({ inputValue }) => { - const customValueOption = - createCustomValue && - inputValue && - items.findIndex((opt) => opt.label === inputValue || opt.value === inputValue) === -1 - ? { - // 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'), - } - : null; - if (isAsync) { - if (customValueOption) { - setItems([customValueOption]); + onInputValueChange: ({ inputValue, isOpen }) => { + if (!isOpen) { + // Prevent stale options from showing on reopen + if (isAsync) { + setItems([], ''); } - setAsyncLoading(true); - debounceAsync(inputValue, customValueOption); + // Otherwise there's nothing else to do when the menu isnt open return; } - const filteredItems = options.filter(itemFilter(inputValue)); + if (!isAsync) { + const filteredItems = options.filter(itemFilter(inputValue)); + setItems(filteredItems, inputValue); + } else { + if (inputValue && createCustomValue) { + setItems([], inputValue); + } - setItems(customValueOption ? [customValueOption, ...filteredItems] : filteredItems); + setAsyncLoading(true); + debounceAsync(inputValue); + } }, onIsOpenChange: ({ isOpen, inputValue }) => { - // Default to displaying all values when opening - if (isOpen && !isAsync) { - setItems(options); - return; - } - - if (isOpen && isAsync) { + // Loading async options mostly happens in onInputValueChange, but if the menu is opened with an empty input + // then onInputValueChange isn't called (because the input value hasn't changed) + if (isAsync && isOpen && inputValue === '') { setAsyncLoading(true); - loadOptions(inputValue ?? '') - .then((options) => { - setItems(options); + // TODO: dedupe this loading logic with debounceAsync + loadOptions(inputValue) + .then((opts) => { + setItems(opts, inputValue); setAsyncLoading(false); setAsyncError(false); }) @@ -230,22 +261,52 @@ export const Combobox = ({ setAsyncLoading(false); } }); - return; } }, + onHighlightedIndexChange: ({ highlightedIndex, type }) => { if (type !== useCombobox.stateChangeTypes.MenuMouseLeave) { rowVirtualizer.scrollToIndex(highlightedIndex); } }, + + stateReducer(state, actionAndChanges) { + let { changes } = actionAndChanges; + const menuBeingOpened = state.isOpen === false && changes.isOpen === true; + const menuBeingClosed = state.isOpen === true && changes.isOpen === false; + + // Reset the input value when the menu is opened. If the menu is opened due to an input change + // then make sure we keep that. + // This will trigger onInputValueChange to load async options + if (menuBeingOpened && changes.inputValue === state.inputValue) { + changes = { + ...changes, + inputValue: '', + }; + } + + if (menuBeingClosed) { + // Flush the selected item to the input when the menu is closed + if (changes.selectedItem) { + changes = { + ...changes, + inputValue: itemToString(changes.selectedItem), + }; + } else if (changes.inputValue !== '') { + // Otherwise if no selected value, clear any search from the input + changes = { + ...changes, + inputValue: '', + }; + } + } + + return changes; + }, }); const { inputRef, floatingRef, floatStyles, scrollRef } = useComboboxFloat(items, rowVirtualizer.range, isOpen); - const onBlur = useCallback(() => { - setInputValue(selectedItem?.label ?? value?.toString() ?? ''); - }, [selectedItem, setInputValue, value]); - const handleSuffixClick = useCallback(() => { isOpen ? closeMenu() : openMenu(); }, [isOpen, openMenu, closeMenu]); @@ -259,6 +320,8 @@ export const Combobox = ({ ? 'search' : 'angle-down'; + const placeholder = (isOpen ? itemToString(selectedItem) : null) || placeholderProp; + return (
({ * See issue here: https://github.com/downshift-js/downshift/issues/718 * Downshift repo: https://github.com/downshift-js/downshift/tree/master */ - onChange: () => {}, - onBlur, + onChange: noop, 'aria-labelledby': ariaLabelledBy, // Label should be handled with the Field component + placeholder, })} />