From 99395b62d4885fdc0d3cf5a4ecc948c67c8878e5 Mon Sep 17 00:00:00 2001 From: Tobias Skarhed <1438972+tskarhed@users.noreply.github.com> Date: Fri, 29 Nov 2024 16:09:38 +0100 Subject: [PATCH] MultiCombobox: Add basic controlled functionality (#97117) * Add basic controlled functionality * Fix controlled component * Make component completely controlled * Fix PR feedback * Add support for number values and values not in options * Fix TS errors * Fix test feedback * Extract function --- .../Combobox/MultiCombobox.internal.story.tsx | 33 ++++-- .../Combobox/MultiCombobox.test.tsx | 99 +++++++++++++++++ .../src/components/Combobox/MultiCombobox.tsx | 102 ++++++++++++++---- .../components/Combobox/OptionListItem.tsx | 7 +- 4 files changed, 214 insertions(+), 27 deletions(-) create mode 100644 packages/grafana-ui/src/components/Combobox/MultiCombobox.test.tsx diff --git a/packages/grafana-ui/src/components/Combobox/MultiCombobox.internal.story.tsx b/packages/grafana-ui/src/components/Combobox/MultiCombobox.internal.story.tsx index d2a5c36eefe..0e3a94b354a 100644 --- a/packages/grafana-ui/src/components/Combobox/MultiCombobox.internal.story.tsx +++ b/packages/grafana-ui/src/components/Combobox/MultiCombobox.internal.story.tsx @@ -1,3 +1,5 @@ +import { action } from '@storybook/addon-actions'; +import { useArgs } from '@storybook/preview-api'; import type { Meta, StoryObj } from '@storybook/react'; import { MultiCombobox } from './MultiCombobox'; @@ -7,17 +9,34 @@ const meta: Meta = { component: MultiCombobox, }; +const commonArgs = { + options: [ + { label: 'Option 1', value: 'option1' }, + { label: 'Option 2', value: 'option2' }, + { label: 'Option 3', value: 'option3' }, + ], + value: ['option2'], + placeholder: 'Select multiple options...', +}; + export default meta; type Story = StoryObj; export const Basic: Story = { - args: { - options: [ - { label: 'Option 1', value: 'option1' }, - { label: 'Option 2', value: 'option2' }, - { label: 'Option 3', value: 'option3' }, - ], - placeholder: 'Select multiple options...', + args: commonArgs, + render: (args) => { + const [{ value }, setArgs] = useArgs(); + + return ( + { + action('onChange')(val); + setArgs({ value: val }); + }} + /> + ); }, }; diff --git a/packages/grafana-ui/src/components/Combobox/MultiCombobox.test.tsx b/packages/grafana-ui/src/components/Combobox/MultiCombobox.test.tsx new file mode 100644 index 00000000000..f0545b13b94 --- /dev/null +++ b/packages/grafana-ui/src/components/Combobox/MultiCombobox.test.tsx @@ -0,0 +1,99 @@ +import { render, screen } from '@testing-library/react'; +import userEvent, { UserEvent } from '@testing-library/user-event'; +import React from 'react'; + +import { MultiCombobox, MultiComboboxProps } from './MultiCombobox'; + +describe('MultiCombobox', () => { + let user: UserEvent; + + beforeEach(() => { + user = userEvent.setup(); + }); + + it('should render with options', async () => { + const options = [ + { label: 'A', value: 'a' }, + { label: 'B', value: 'b' }, + { label: 'C', value: 'c' }, + ]; + render(); + const input = screen.getByRole('combobox'); + user.click(input); + expect(await screen.findByText('A')).toBeInTheDocument(); + expect(await screen.findByText('B')).toBeInTheDocument(); + expect(await screen.findByText('C')).toBeInTheDocument(); + }); + + it('should render with value', () => { + const options = [ + { label: 'A', value: 'a' }, + { label: 'B', value: 'b' }, + { label: 'C', value: 'c' }, + ]; + render(); + expect(screen.getByText('A')).toBeInTheDocument(); + }); + + it('should render with placeholder', () => { + const options = [ + { label: 'A', value: 'a' }, + { label: 'B', value: 'b' }, + { label: 'C', value: 'c' }, + ]; + render(); + expect(screen.getByPlaceholderText('Select')).toBeInTheDocument(); + }); + + it.each([ + ['a', 'b', 'c'], + [1, 2, 3], + ])('should call onChange with the correct values', async (first, second, third) => { + const options = [ + { label: 'A', value: first }, + { label: 'B', value: second }, + { label: 'C', value: third }, + ]; + const onChange = jest.fn(); + + const ControlledMultiCombobox = (props: MultiComboboxProps) => { + const [value, setValue] = React.useState([]); + return ( + { + //@ts-expect-error Don't do this for real life use cases + setValue(val ?? []); + onChange(val); + }} + /> + ); + }; + render(); + const input = screen.getByRole('combobox'); + await user.click(input); + await user.click(await screen.findByRole('option', { name: 'A' })); + + //Second option + await user.click(screen.getByRole('option', { name: 'C' })); + + //Deselect + await user.click(screen.getByRole('option', { name: 'A' })); + + expect(onChange).toHaveBeenNthCalledWith(1, [first]); + expect(onChange).toHaveBeenNthCalledWith(2, [first, third]); + expect(onChange).toHaveBeenNthCalledWith(3, [third]); + }); + + it('should be able to render a valie that is not in the options', async () => { + const options = [ + { label: 'A', value: 'a' }, + { label: 'B', value: 'b' }, + { label: 'C', value: 'c' }, + ]; + render(); + await user.click(screen.getByRole('combobox')); + expect(await screen.findByText('d')).toBeInTheDocument(); + }); +}); diff --git a/packages/grafana-ui/src/components/Combobox/MultiCombobox.tsx b/packages/grafana-ui/src/components/Combobox/MultiCombobox.tsx index f7d8d7de3bc..a8b769ad647 100644 --- a/packages/grafana-ui/src/components/Combobox/MultiCombobox.tsx +++ b/packages/grafana-ui/src/components/Combobox/MultiCombobox.tsx @@ -1,5 +1,5 @@ import { useCombobox, useMultipleSelection } from 'downshift'; -import { useState } from 'react'; +import { useCallback, useMemo, useState } from 'react'; import { useStyles2 } from '../../themes'; import { Checkbox } from '../Forms/Checkbox'; @@ -11,22 +11,34 @@ import { ValuePill } from './ValuePill'; import { getMultiComboboxStyles } from './getMultiComboboxStyles'; interface MultiComboboxBaseProps extends Omit, 'value' | 'onChange'> { - value?: string | Array>; - onChange: (items?: Array>) => void; + value?: T[] | Array>; + onChange: (items?: T[]) => void; } -type MultiComboboxProps = MultiComboboxBaseProps & AutoSizeConditionals; +export type MultiComboboxProps = MultiComboboxBaseProps & AutoSizeConditionals; export const MultiCombobox = (props: MultiComboboxProps) => { - const { options, placeholder } = props; + const { options, placeholder, onChange, value } = props; + const isAsync = typeof options === 'function'; + + const selectedItems = useMemo(() => { + if (!value || isAsync) { + //TODO handle async + return []; + } + + return getSelectedItemsFromValue(value, options); + }, [value, options, isAsync]); const multiStyles = useStyles2(getMultiComboboxStyles); - const isAsync = typeof options === 'function'; - const [items, _baseSetItems] = useState(isAsync ? [] : options); const [isOpen, setIsOpen] = useState(false); - const [selectedItems, setSelectedItems] = useState>>([]); + + const isOptionSelected = useCallback( + (item: ComboboxOption) => selectedItems.some((opt) => opt.value === item.value), + [selectedItems] + ); const [inputValue, setInputValue] = useState(''); @@ -39,9 +51,10 @@ export const MultiCombobox = (props: MultiComboboxPro case useMultipleSelection.stateChangeTypes.DropdownKeyDownBackspace: case useMultipleSelection.stateChangeTypes.FunctionRemoveSelectedItem: if (newSelectedItems) { - setSelectedItems(newSelectedItems); + onChange(getComboboxOptionsValues(newSelectedItems)); } break; + default: break; } @@ -55,26 +68,37 @@ export const MultiCombobox = (props: MultiComboboxPro getInputProps, highlightedIndex, getItemProps, - //selectedItem, } = useCombobox({ isOpen, items, itemToString, inputValue, - //defaultHighlightedIndex: 0, selectedItem: null, + stateReducer: (state, actionAndChanges) => { + const { changes, type } = actionAndChanges; + switch (type) { + case useCombobox.stateChangeTypes.InputKeyDownEnter: + case useCombobox.stateChangeTypes.ItemClick: + return { + ...changes, + isOpen: true, + defaultHighlightedIndex: 0, + }; + default: + return changes; + } + }, onStateChange: ({ inputValue: newInputValue, type, selectedItem: newSelectedItem }) => { switch (type) { case useCombobox.stateChangeTypes.InputKeyDownEnter: case useCombobox.stateChangeTypes.ItemClick: if (newSelectedItem) { - const isAlreadySelected = selectedItems.some((opt) => opt.value === newSelectedItem.value); - if (!isAlreadySelected) { - setSelectedItems([...selectedItems, newSelectedItem]); + if (!isOptionSelected(newSelectedItem)) { + onChange(getComboboxOptionsValues([...selectedItems, newSelectedItem])); break; } - removeSelectedItem(newSelectedItem); + removeSelectedItem(newSelectedItem); // onChange is handled by multiselect here } break; case useCombobox.stateChangeTypes.InputBlur: @@ -115,7 +139,8 @@ export const MultiCombobox = (props: MultiComboboxPro
{items.map((item, index) => { const itemProps = getItemProps({ item, index }); - const isSelected = selectedItems.some((opt) => opt.value === item.value); + const isSelected = isOptionSelected(item); + const id = 'multicombobox-option-' + item.value.toString(); return (
  • (props: MultiComboboxPro > {' '} {/* Add styling with virtualization */} - - + +
    • ); })} @@ -136,3 +161,44 @@ export const MultiCombobox = (props: MultiComboboxPro
    ); }; + +function getSelectedItemsFromValue( + value: T[] | Array>, + options: Array> +) { + if (!isComboboxOptions(value)) { + const resultingItems: Array | undefined> = []; + + for (const item of options) { + for (const [index, val] of value.entries()) { + if (val === item.value) { + resultingItems[index] = item; + } + } + if (resultingItems.length === value.length && !resultingItems.includes(undefined)) { + // We found all items for the values + break; + } + } + + // Handle values that are not in options + for (const [index, val] of value.entries()) { + if (resultingItems[index] === undefined) { + resultingItems[index] = { value: val }; + } + } + return resultingItems.filter((item) => item !== undefined); // TODO: Not actually needed, but TS complains + } + + return value; +} + +function isComboboxOptions( + value: T[] | Array> +): value is Array> { + return typeof value[0] === 'object'; +} + +function getComboboxOptionsValues(optionArray: Array>) { + return optionArray.map((option) => option.value); +} diff --git a/packages/grafana-ui/src/components/Combobox/OptionListItem.tsx b/packages/grafana-ui/src/components/Combobox/OptionListItem.tsx index 1138a60c282..d80c70e01d2 100644 --- a/packages/grafana-ui/src/components/Combobox/OptionListItem.tsx +++ b/packages/grafana-ui/src/components/Combobox/OptionListItem.tsx @@ -5,13 +5,16 @@ import { getComboboxStyles } from './getComboboxStyles'; interface Props { option: ComboboxOption; + id: string; } -export const OptionListItem = ({ option }: Props) => { +export const OptionListItem = ({ option, id }: Props) => { const styles = useStyles2(getComboboxStyles); return (
    - {option.label ?? option.value} + + {option.label ?? option.value} + {option.description && {option.description}}
    );