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

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<typeof MultiCombobox> = {
   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<typeof MultiCombobox>;
 
 export const Basic: Story = {
-  args: {
+  args: commonArgs,
-    options: [
+  render: (args) => {
-      { label: 'Option 1', value: 'option1' },
+    const [{ value }, setArgs] = useArgs();
-      { label: 'Option 2', value: 'option2' },
+
-      { label: 'Option 3', value: 'option3' },
+    return (
-    ],
+      <MultiCombobox
-    placeholder: 'Select multiple options...',
+        {...args}
+        value={value}
+        onChange={(val) => {
+          action('onChange')(val);
+          setArgs({ value: val });
+        }}
+      />
+    );
   },
 };

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(<MultiCombobox options={options} value={[]} onChange={jest.fn()} />);
+    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(<MultiCombobox options={options} value={['a']} onChange={jest.fn()} />);
+    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(<MultiCombobox options={options} value={[]} onChange={jest.fn()} placeholder="Select" />);
+    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<string | number>) => {
+      const [value, setValue] = React.useState<string[] | number[]>([]);
+      return (
+        <MultiCombobox
+          {...props}
+          value={value}
+          onChange={(val) => {
+            //@ts-expect-error Don't do this for real life use cases
+            setValue(val ?? []);
+            onChange(val);
+          }}
+        />
+      );
+    };
+    render(<ControlledMultiCombobox options={options} value={[]} onChange={onChange} />);
+    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(<MultiCombobox options={options} value={['a', 'd', 'c']} onChange={jest.fn()} />);
+    await user.click(screen.getByRole('combobox'));
+    expect(await screen.findByText('d')).toBeInTheDocument();
+  });
+});

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<T extends string | number> extends Omit<ComboboxBaseProps<T>, 'value' | 'onChange'> {
-  value?: string | Array<ComboboxOption<T>>;
+  value?: T[] | Array<ComboboxOption<T>>;
-  onChange: (items?: Array<ComboboxOption<T>>) => void;
+  onChange: (items?: T[]) => void;
 }
 
-type MultiComboboxProps<T extends string | number> = MultiComboboxBaseProps<T> & AutoSizeConditionals;
+export type MultiComboboxProps<T extends string | number> = MultiComboboxBaseProps<T> & AutoSizeConditionals;
 
 export const MultiCombobox = <T extends string | number>(props: MultiComboboxProps<T>) => {
-  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<T>(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<Array<ComboboxOption<T>>>([]);
+
+  const isOptionSelected = useCallback(
+    (item: ComboboxOption<T>) => selectedItems.some((opt) => opt.value === item.value),
+    [selectedItems]
+  );
 
   const [inputValue, setInputValue] = useState('');
 
@@ -39,9 +51,10 @@ export const MultiCombobox = <T extends string | number>(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 = <T extends string | number>(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 (!isOptionSelected(newSelectedItem)) {
-            if (!isAlreadySelected) {
+              onChange(getComboboxOptionsValues([...selectedItems, newSelectedItem]));
-              setSelectedItems([...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 = <T extends string | number>(props: MultiComboboxPro
             <div>
               {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 (
                   <li
                     key={item.value}
@@ -124,8 +149,8 @@ export const MultiCombobox = <T extends string | number>(props: MultiComboboxPro
                   >
                     {' '}
                     {/* Add styling with virtualization */}
-                    <Checkbox key={`${item.value}${index}`} value={isSelected} />
+                    <Checkbox key={id} value={isSelected} aria-labelledby={id} />
-                    <OptionListItem option={item} />
+                    <OptionListItem option={item} id={id} />
                   </li>
                 );
               })}
@@ -136,3 +161,44 @@ export const MultiCombobox = <T extends string | number>(props: MultiComboboxPro
     </div>
   );
 };
+
+function getSelectedItemsFromValue<T extends string | number>(
+  value: T[] | Array<ComboboxOption<T>>,
+  options: Array<ComboboxOption<T>>
+) {
+  if (!isComboboxOptions(value)) {
+    const resultingItems: Array<ComboboxOption<T> | 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<T extends string | number>(
+  value: T[] | Array<ComboboxOption<T>>
+): value is Array<ComboboxOption<T>> {
+  return typeof value[0] === 'object';
+}
+
+function getComboboxOptionsValues<T extends string | number>(optionArray: Array<ComboboxOption<T>>) {
+  return optionArray.map((option) => option.value);
+}

packages/grafana-ui/src/components/Combobox/OptionListItem.tsx

@@ -5,13 +5,16 @@ import { getComboboxStyles } from './getComboboxStyles';
 
 interface Props {
   option: ComboboxOption<string | number>;
+  id: string;
 }
 
-export const OptionListItem = ({ option }: Props) => {
+export const OptionListItem = ({ option, id }: Props) => {
   const styles = useStyles2(getComboboxStyles);
   return (
     <div className={styles.optionBody}>
-      <span className={styles.optionLabel}>{option.label ?? option.value}</span>
+      <span className={styles.optionLabel} id={id}>
+        {option.label ?? option.value}
+      </span>
       {option.description && <span className={styles.optionDescription}>{option.description}</span>}
     </div>
   );