fix(combobox): controlled combobox clear (carbon-design-system#18041)

* fix(combobox): controlled combobox clear * update the way onChange is called in comboBox to prevent unwanted calls * add unit test cases for controlled combobox managed by parent component * fixed format * fix(combobox): console errors in combobox * fix withLayer proptypes * add required onChange prop to examples in ComboBox.mdx --------- Co-authored-by: Taylor Jones <[email protected]> Co-authored-by: Guilherme Datilio Ribeiro <[email protected]>
guidari · Nov 14, 2024 · 1009bab · 1009bab
1 parent ad23c63
commit 1009bab
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 22 deletions.
diff --git a/packages/react/.storybook/templates/WithLayer/index.js b/packages/react/.storybook/templates/WithLayer/index.js
@@ -48,7 +48,7 @@ WithLayer.propTypes = {
    * Can be either a node or a function that receives the layer
    * index as a parameter and returns the child for that layer.
    */
-  children: PropTypes.oneOf([PropTypes.node, PropTypes.func]),
+  children: PropTypes.oneOfType([PropTypes.node, PropTypes.func]),
 };
 
 export { WithLayer };
diff --git a/packages/react/src/components/ComboBox/ComboBox-test.js b/packages/react/src/components/ComboBox/ComboBox-test.js
@@ -46,9 +46,10 @@ const ControlledComboBox = ({ controlledItem }) => {
         placeholder="Filter..."
         type="default"
       />
-      <div>value: {value.label}</div>
+      <div>value: {value?.label || 'none'}</div>
       <div>onChangeCallCount: {onChangeCallCount}</div>
-      <button onClick={() => setValue(items[2])}>Choose item 3</button>
+      <button onClick={() => setValue(items[3])}>Choose item 3</button>
+      <button onClick={() => setValue(null)}>reset</button>
     </div>
   );
 };
@@ -366,6 +367,55 @@ describe('ComboBox', () => {
         screen.getByRole('combobox', { value: 'Item 2' })
       ).toBeInTheDocument();
     });
+    it('should update and call `onChange` once when selection is cleared from the combobox and the external state managing selectedItem is updated', async () => {
+      render(<ControlledComboBox />);
+      expect(screen.getByText('onChangeCallCount: 0')).toBeInTheDocument();
+      await openMenu();
+      await userEvent.click(screen.getByRole('option', { name: 'Item 2' }));
+      expect(screen.getByText('onChangeCallCount: 1')).toBeInTheDocument();
+      await userEvent.click(
+        screen.getByRole('button', { name: 'Clear selected item' })
+      );
+      expect(screen.getByText('onChangeCallCount: 2')).toBeInTheDocument();
+      expect(screen.getByText('value: none')).toBeInTheDocument();
+      expect(findInputNode()).toHaveDisplayValue('');
+    });
+    it('should update and call `onChange` once when selection is cleared from the combobox after an external update is made, and the external state managing selectedItem is updated', async () => {
+      render(<ControlledComboBox />);
+      expect(screen.getByText('onChangeCallCount: 0')).toBeInTheDocument();
+      await openMenu();
+      await userEvent.click(
+        screen.getByRole('button', { name: 'Choose item 3' })
+      );
+      expect(screen.getByText('onChangeCallCount: 1')).toBeInTheDocument();
+      await userEvent.click(
+        screen.getByRole('button', { name: 'Clear selected item' })
+      );
+      expect(screen.getByText('onChangeCallCount: 2')).toBeInTheDocument();
+      expect(screen.getByText('value: none')).toBeInTheDocument();
+      expect(findInputNode()).toHaveDisplayValue('');
+    });
+    it('should update and call `onChange` when a combination of external and combobox selections are made', async () => {
+      render(<ControlledComboBox />);
+      expect(screen.getByText('onChangeCallCount: 0')).toBeInTheDocument();
+      await userEvent.click(
+        screen.getByRole('button', { name: 'Choose item 3' })
+      );
+      expect(screen.getByText('onChangeCallCount: 1')).toBeInTheDocument();
+      expect(findInputNode()).toHaveDisplayValue('Item 3');
+      expect(screen.getByText('value: Item 3')).toBeInTheDocument();
+      await openMenu();
+      await userEvent.click(screen.getByRole('option', { name: 'Item 2' }));
+      expect(screen.getByText('onChangeCallCount: 2')).toBeInTheDocument();
+      expect(findInputNode()).toHaveDisplayValue('Item 2');
+      expect(screen.getByText('value: Item 2')).toBeInTheDocument();
+      await userEvent.click(
+        screen.getByRole('button', { name: 'Clear selected item' })
+      );
+      expect(screen.getByText('onChangeCallCount: 3')).toBeInTheDocument();
+      expect(screen.getByText('value: none')).toBeInTheDocument();
+      expect(findInputNode()).toHaveDisplayValue('');
+    });
     it('should update and call `onChange` once when selection is updated externally', async () => {
       const { rerender } = render(
         <ComboBox {...mockProps} selectedItem={mockProps.items[0]} />
@@ -375,13 +425,14 @@ describe('ComboBox', () => {
       expect(findInputNode()).toHaveDisplayValue(mockProps.items[1].label);
       expect(mockProps.onChange).toHaveBeenCalledTimes(1);
     });
-    it('should clear selected item and call `onChange` when selection is cleared from the combobox', async () => {
-      render(<ComboBox {...mockProps} selectedItem={mockProps.items[1]} />);
-      expect(mockProps.onChange).not.toHaveBeenCalled();
-      await userEvent.click(
-        screen.getByRole('button', { name: 'Clear selected item' })
-      );
-      expect(mockProps.onChange).toHaveBeenCalledTimes(1);
+    it('should clear selected item and call `onChange` when selection is cleared externally', async () => {
+      render(<ControlledComboBox />);
+      expect(screen.getByText('onChangeCallCount: 0')).toBeInTheDocument();
+      await openMenu();
+      await userEvent.click(screen.getByRole('option', { name: 'Item 2' }));
+      await userEvent.click(screen.getByRole('button', { name: 'reset' }));
+      expect(screen.getByText('onChangeCallCount: 2')).toBeInTheDocument();
+      expect(screen.getByText('value: none')).toBeInTheDocument();
       expect(findInputNode()).toHaveDisplayValue('');
     });
     it('should clear selected item when `selectedItem` is updated to `null` externally', async () => {

diff --git a/packages/react/src/components/ComboBox/ComboBox.mdx b/packages/react/src/components/ComboBox/ComboBox.mdx
@@ -130,7 +130,7 @@ want to fully control the component, you can use `initialSelectedItem`
 ```jsx
 const items = ['Option 1', 'Option 2', 'Option 3']
 
-<Combobox initialSelectedItem={items[2]} />
+<Combobox initialSelectedItem={items[2]} onChange={() => {}} />
 ```
 
 ## `selectedItem`
@@ -195,6 +195,7 @@ dropdown item in a custom element.
 
 ```jsx
 <Combobox
+  onChange={() => {}}
   items={[
     { id: 'option-0', text: 'Option 0' },
     { id: 'option-1', text: 'Option 1' },
@@ -226,6 +227,7 @@ If the `items` array is not an array of strings, you'll need to use
     { id: 'option-2', text: 'Option 2' },
   ]}
   itemToString={(item) => (item ? item.text : '')}
+  onChange={() => {}}
 />
 ```
 
@@ -244,6 +246,7 @@ prop.
   shouldFilterItem={(menu) =>
     menu?.item?.toLowerCase().includes(menu?.inputValue?.toLowerCase())
   }
+  onChange={() => {}}
 />
 
 ```jsx
@@ -256,6 +259,7 @@ const filterItems = (menu) => {
   helperText="Combobox helper text"
   items={['Apple', 'Orange', 'Banana']}
   shouldFilterItem={filterItems}
+  onChange={() => {}}
 />;
 ```
 

diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx
@@ -510,7 +510,6 @@ const ComboBox = forwardRef(
           selectedItem: selectedItemProp,
           prevSelectedItem: prevSelectedItemProp.current,
         });
-
         // selectedItem has been updated externally, need to update state and call onChange
         if (inputValue !== currentInputValue) {
           setInputValue(currentInputValue);
@@ -744,18 +743,8 @@ const ComboBox = forwardRef(
       onInputValueChange({ inputValue }) {
         const normalizedInput = inputValue || '';
         setInputValue(normalizedInput);
-        if (selectedItemProp && !inputValue) {
-          // ensure onChange is called when selectedItem is cleared
-          onChange({ selectedItem, inputValue: normalizedInput });
-        }
         setHighlightedIndex(indexToHighlight(normalizedInput));
       },
-      onSelectedItemChange({ selectedItem }) {
-        // only call onChange if new selection is updated from previous
-        if (!isEqual(selectedItem, selectedItemProp)) {
-          onChange({ selectedItem });
-        }
-      },
       onHighlightedIndexChange: ({ highlightedIndex }) => {
         if (highlightedIndex! > -1 && typeof window !== undefined) {
           const itemArray = document.querySelectorAll(
@@ -770,6 +759,20 @@ const ComboBox = forwardRef(
           }
         }
       },
+      onStateChange: ({ type, selectedItem: newSelectedItem }) => {
+        if (
+          type === '__item_click__' &&
+          !isEqual(selectedItemProp, newSelectedItem)
+        ) {
+          onChange({ selectedItem: newSelectedItem });
+        }
+        if (
+          type === '__function_select_item__' ||
+          type === '__input_keydown_enter__'
+        ) {
+          onChange({ selectedItem: newSelectedItem });
+        }
+      },
       initialSelectedItem: initialSelectedItem,
       inputId: id,
       stateReducer,