From 1009bab754beee911bbf6b508b584c6d78d4d9e5 Mon Sep 17 00:00:00 2001 From: Noah Sgorbati Date: Thu, 14 Nov 2024 17:13:38 +0000 Subject: [PATCH] fix(combobox): controlled combobox clear (#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 Co-authored-by: Guilherme Datilio Ribeiro --- .../.storybook/templates/WithLayer/index.js | 2 +- .../src/components/ComboBox/ComboBox-test.js | 69 ++++++++++++++++--- .../src/components/ComboBox/ComboBox.mdx | 6 +- .../src/components/ComboBox/ComboBox.tsx | 25 ++++--- 4 files changed, 80 insertions(+), 22 deletions(-) diff --git a/packages/react/.storybook/templates/WithLayer/index.js b/packages/react/.storybook/templates/WithLayer/index.js index 248003d7ee3f..a92dd9176b46 100644 --- 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 index a1916be39a4c..15576ee7c555 100644 --- 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" /> -
value: {value.label}
+
value: {value?.label || 'none'}
onChangeCallCount: {onChangeCallCount}
- + + ); }; @@ -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(); + 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(); + 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(); + 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( @@ -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(); - 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(); + 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 index df7dba08a568..d5dfa4adfee2 100644 --- 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'] - + {}} /> ``` ## `selectedItem` @@ -195,6 +195,7 @@ dropdown item in a custom element. ```jsx {}} 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 index 928c394036a3..00c3a5485ec5 100644 --- 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,