feat: ComboBox component #694
Conversation
haworku
requested review from
ahobson,
gidjin,
sarboc,
suzubara and
tinyels
as code owners
There was a problem hiding this comment.
wow, this is an incredibly impressive PR and v1 of this component 👏 !! I think it is really solid and fine to follow up on the outstanding bugs as additional issues after merging. I left some comments, mostly noting differences between the tests and my UX manually testing in Chrome, and some questions.
The only thing I would consider a blocker is that I am seeing the selected value clear out whenever I blur, making it impossible to actually persist a value in the form (see gif). if we can fix that then I think this is good to go!
|
|
||
| userEvent.click(getByTestId('combo-box-clear-button')) | ||
| fireEvent.blur(getByTestId('combo-box-clear-button')) | ||
| expect(getByTestId('combo-box-input')).toHaveFocus() |
There was a problem hiding this comment.
should the clear button be tabbable? I wasn't able to in chrome
There was a problem hiding this comment.
Yes. This is documented in a failing test.
Update: Fixed in fff5b90
| key: 'ArrowDown', | ||
| }) | ||
|
|
||
| expect(getByTestId('combo-box-option-yuzu')).toHaveFocus() |
There was a problem hiding this comment.
fwiw, in Chrome when I press the down arrow after the last option it does not change focus
There was a problem hiding this comment.
Yes. This is a failing test.
Update: Fixed in 0cba14f
| }) | ||
|
|
||
| expect(getByTestId('combo-box-option-apple')).toHaveFocus() | ||
| expect(getByTestId('combo-box-option-list')).toBeVisible() |
There was a problem hiding this comment.
this is not the behavior I'm seeing.. in Chrome when I have selected Apricot and the Apple option is focused and I press the up arrow, the menu closes
There was a problem hiding this comment.
🐛 This is a bug and this test should be failing. Changing fireEvent.focus to userEvent.hover(getByTestId('combo-box-option-apple')) makes it fail as it should. Great catch! 🌮
I'm gonna go through the tests and replace all instances of fireEvent.focus with userEvent.hover where relevant. In almost all cases that is the user behavior we are testing.
Update: fixed in 0cba14f
| expect(getByTestId('combo-box-option-list')).toBeVisible() | ||
| }) | ||
|
|
||
| it('hides options list when clicking away', () => { |
There was a problem hiding this comment.
should the options list be hidden if an option has focus and you click away? it hides when the input has focus and I click away but not if I've hovered over one of the options first
There was a problem hiding this comment.
🐛 It should be hidden in either case when you click away. That's a bug, will fix and write a test.
Update: fixed in 2bc43a2
| userEvent.type(input, 'yu') | ||
| userEvent.click(getByTestId('combo-box-option-yuzu')) | ||
|
|
||
| expect(getByTestId('combo-box-input')).toHaveValue('Yuzu') |
There was a problem hiding this comment.
It might be worth testing that the onChange handler was called with the value here
…to be fixed for merge.
- does not change focus if last option focused, arrowDown pressed - stays open when option selected, first option focused, arrowUp pressed - replace fireEvent.focus w/ userEvents to ensure tests work as expected
- tab once with selected option focuses the clear button - tab twice with default value, and list open, focuses selected option
There was a problem hiding this comment.
Verified that the fixes look great in Storybook! They don't seem to be reflected in the example app but that is an existing issue. This is awesome!! 🚢 🚀 🔥 🎉
(I took a look at the Happo diffs and they look fine, maybe will be resolved once this branch is updated from main or they could be a result of Happo updating their browsers)
Summary
USWDS component: https://designsystem.digital.gov/form-controls/03-combo-box/
Current Status:
ComboBox.tsxandComboBox.test.tsx.ComboBox.stories.tsx) and implementation in example app (/examplefiles).Related Issues or PRs
closes #170
How To Test
Screenshots (optional)