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: Added hook to combobox for when options change #2715

Merged
merged 10 commits into from Mar 7, 2024
44 changes: 37 additions & 7 deletions src/components/forms/ComboBox/ComboBox.stories.tsx
@@ -1,11 +1,11 @@
import React, { useRef } from 'react'
import React, { useRef, useState } from 'react'

import { ComboBox, ComboBoxRef } from './ComboBox'
import { Form } from '../Form/Form'
import { Label } from '../Label/Label'
import { TextInput } from '../TextInput/TextInput'
import { Button } from '../../Button/Button'
import { fruits } from './fruits'
import { fruits, veggies } from './foods'
import { Radio } from '../Radio/Radio'

export default {
title: 'Components/Combo box',
Expand Down Expand Up @@ -112,12 +112,42 @@ export const WithOtherFields = (): React.ReactElement => {
label: key,
}))

const veggieList = Object.entries(veggies).map(([value, key]) => ({
value: value,
label: key,
}))

const [options, setOptions] = useState(fruitList)

const ref = useRef<ComboBoxRef>(null)
const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
ref.current?.clearSelection()
const selection = e.target.id
setOptions(selection === 'fruit' ? fruitList : veggieList)
}

return (
<Form onSubmit={noop}>
<Label htmlFor="fruit">Select a fruit</Label>
<ComboBox id="fruit" name="fruit" options={fruitList} onChange={noop} />
<Label htmlFor="fruitDescription">Description</Label>
<TextInput id="fruitDescription" name="fruitDescription" type="text" />
<Label htmlFor="food">Select a group</Label>
<Radio
name="food"
id="fruit"
label="Fruits"
onChange={handleChange}
defaultChecked></Radio>
<Radio
name="food"
id="veggie"
label="Vegetables"
onChange={handleChange}></Radio>
<Label htmlFor="food">Select a food</Label>
<ComboBox
id="fruit"
name="fruit"
options={options}
onChange={noop}
ref={ref}
/>
</Form>
)
}
Expand Down
39 changes: 33 additions & 6 deletions src/components/forms/ComboBox/ComboBox.test.tsx
Expand Up @@ -2,20 +2,29 @@ import React from 'react'
import { screen, render, waitFor } from '@testing-library/react'
import { userEvent } from '@testing-library/user-event'

import { ComboBox, ComboBoxRef } from './ComboBox'
import { ComboBox, ComboBoxOption, ComboBoxRef } from './ComboBox'
import { TextInput } from '../TextInput/TextInput'
import { fruits } from './fruits'
import { fruits, veggies } from './foods'

/*
Source of truth for combo box behavior is USWDS storybook examples and tests. For more:
- https://designsystem.digital.gov/form-controls/03-combo-box/
- https://github.com/uswds/uswds/tree/7a89611fe649650922e4d431b78c39fed6a867e1/spec/unit/combo-box
*/

const fruitOptions = Object.entries(fruits).map(([value, key]) => ({
value: value,
label: key,
}))
const fruitOptions: ComboBoxOption[] = Object.entries(fruits).map(
([value, key]) => ({
value: value,
label: key,
})
)

const veggieOptions: ComboBoxOption[] = Object.entries(veggies).map(
([value, key]) => ({
value: value,
label: key,
})
)

describe('ComboBox component', () => {
it('renders the expected markup without errors', () => {
Expand Down Expand Up @@ -87,6 +96,24 @@ describe('ComboBox component', () => {
expect(comboBoxInput).toHaveValue('Avocado')
})

it('updates options when prop changes', async () => {
const Wrapper = (props: { options: ComboBoxOption[] }) => {
return (
<ComboBox
id="favorite-fruit"
name="favorite-fruit"
options={props.options}
onChange={vi.fn()}
/>
)
}
const { rerender } = render(<Wrapper options={fruitOptions} />)
const comboBoxSelect = screen.getByTestId('combo-box-select')
expect(comboBoxSelect).toHaveValue(fruitOptions[0].value)
rerender(<Wrapper options={veggieOptions} />)
expect(comboBoxSelect).toHaveValue(veggieOptions[0].value)
})

describe('toggling the list', () => {
it('renders all options when the list is open', async () => {
const fruitAbridged = fruitOptions.slice(0, 3)
Expand Down
4 changes: 4 additions & 0 deletions src/components/forms/ComboBox/ComboBox.tsx
Expand Up @@ -146,6 +146,10 @@ const ComboBoxForwardRef: React.ForwardRefRenderFunction<
const listRef = useRef<HTMLUListElement>(null)
const focusedItemRef = useRef<HTMLLIElement>(null)

useEffect(() => {
state.filteredOptions = options
}, [options])
Comment on lines +149 to +151
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it would clobber the filters in place if the user has already typed into the combo box. Haven't tested myself, but wanted to raise.

Combo box is tricky because it uses a reducer, in this case specifically the ActionTypes.UPDATE_FILTER handler

Copy link
Contributor

Choose a reason for hiding this comment

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

case ActionTypes.UPDATE_FILTER: {
const { closestMatch, optionsToDisplay } = getPotentialMatches(
action.value
)
const newState = {
...state,
isOpen: true,
filteredOptions: optionsToDisplay,
inputValue: action.value,
statusText: `${optionsToDisplay.length} result${
optionsToDisplay.length > 1 ? 's' : ''
} available.`,
}
if (optionsToDisplay.length < 1) {
newState.statusText = 'No results.'
}
if (disableFiltering || !state.selectedOption) {
newState.focusedOption = closestMatch
} else if (state.selectedOption) {
if (newState.filteredOptions.includes(state.selectedOption)) {
newState.focusedOption = state.selectedOption
} else {
newState.focusedOption = closestMatch
}
}
return newState
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this useEffect should dispatch an event that both changes the options to filter from, and filters the list if the user has already typed into the combo box when the options change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so... e.g. I added a little 'test' console log right above the dispatch:

dispatch({ type: ActionTypes.UPDATE_FILTER, value: e.target.value })

And it doesn't seem to fire when the options change, just when the input is typed in:
Feb-07-2024 10-07-17

Copy link
Contributor

Choose a reason for hiding this comment

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

Finally got around to testing this. Its not what I expected (forgot that blurring the combobox clears user input, so less of a concern to filter the list based on what the user has already typed)

But we should probably at least clear the selected item, especially if the selection is not a possible choice after the options change:

combobox

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same thought, but left it off to preserve the simplicity of the example's focus even if impractical for actual use. Also, adding a ref-based clear on the radio change also prompts the options to change without needing the useEffect, but I figured that shouldn't be the ideal fix here. So I'll add the clear and keep the useEffect too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feb-07-2024 16-45-22


useEffect(() => {
onChange && onChange(state.selectedOption?.value || undefined)
}, [state.selectedOption])
Expand Down
Expand Up @@ -64,3 +64,9 @@ export const fruits = {
'white currant': 'White currant',
yuzu: 'Yuzu',
}

export const veggies = {
celery: 'Celery',
onion: 'Onion',
pepper: 'Pepper',
}