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

Conversation

werdnanoslen
Copy link
Contributor

Summary

  • added hook to combobox for when options change
  • added test
  • updated story to use this feature
  • added veggies to food list and renamed file

Related Issues or PRs

Resolves #2182

How To Test

  • Run new test
  • See updated story

Screenshots (optional)

Jan-19-2024 10-59-17

- added test
- updated story to use this feature
- added veggies to food list and renamed file
@werdnanoslen werdnanoslen requested review from a team as code owners January 19, 2024 19:00
@werdnanoslen
Copy link
Contributor Author

This may cause new behaviors, so might be a breaking change?

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

@brandonlenz brandonlenz Jan 29, 2024

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
Contributor 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

@brandonlenz brandonlenz Feb 8, 2024

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
Contributor 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
Contributor 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

Copy link
Contributor

@shkeating shkeating left a comment

Choose a reason for hiding this comment

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

looks good to me!

@shkeating shkeating merged commit 031b9fb into main Mar 7, 2024
7 checks passed
@shkeating shkeating deleted the an-combooptions-2182 branch March 7, 2024 17:16
@shkeating
Copy link
Contributor

oop @brandonlenz i didn't realize this was still waiting on review from you 😳 i didn't know it would let me merge without code owners review

jpandersen87 pushed a commit to jpandersen87/react-uswds that referenced this pull request Mar 18, 2024
Co-authored-by: Shauna Keating <59394696+shkeating@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fix] when ComboBox options prop is updated displayed options do not change
3 participants