Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(combobox): controlled combobox clear #18041

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react/.storybook/templates/WithLayer/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
69 changes: 60 additions & 9 deletions packages/react/src/components/ComboBox/ComboBox-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>
);
};
Expand Down Expand Up @@ -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]} />
Expand All @@ -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 () => {
Expand Down
6 changes: 5 additions & 1 deletion packages/react/src/components/ComboBox/ComboBox.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -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' },
Expand Down Expand Up @@ -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={() => {}}
/>
```

Expand All @@ -244,6 +246,7 @@ prop.
shouldFilterItem={(menu) =>
menu?.item?.toLowerCase().includes(menu?.inputValue?.toLowerCase())
}
onChange={() => {}}
/>

```jsx
Expand All @@ -256,6 +259,7 @@ const filterItems = (menu) => {
helperText="Combobox helper text"
items={['Apple', 'Orange', 'Banana']}
shouldFilterItem={filterItems}
onChange={() => {}}
/>;
```

Expand Down
25 changes: 14 additions & 11 deletions packages/react/src/components/ComboBox/ComboBox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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(
Expand All @@ -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__' ||

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use enum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is already merged, and a fix for this has also been merged, see #18145

type === '__input_keydown_enter__'
) {
onChange({ selectedItem: newSelectedItem });
}
},
initialSelectedItem: initialSelectedItem,
inputId: id,
stateReducer,
Expand Down
Loading