-
Notifications
You must be signed in to change notification settings - Fork 80
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
Conversation
- added test - updated story to use this feature - added veggies to food list and renamed file
This may cause new behaviors, so might be a breaking change? |
useEffect(() => { | ||
state.filteredOptions = options | ||
}, [options]) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-uswds/src/components/forms/ComboBox/useComboBox.ts
Lines 108 to 138 in 5457bba
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 | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
react-uswds/src/components/forms/ComboBox/ComboBox.tsx
Lines 399 to 400 in 9ac7a45
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:
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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!
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 |
Co-authored-by: Shauna Keating <59394696+shkeating@users.noreply.github.com>
Summary
Related Issues or PRs
Resolves #2182
How To Test
Screenshots (optional)