Rework combobox.jsx to functional component #3854
Conversation
This is barebones features of the combobox, but written as a functional component. It features a text input and a div containing a list of buttons with the different options included. It functionally updates the brew metadata props and loads them when the component initializes. Many features of the original combobox are still missing. Light re-styling. Small HTML changes to metadataEditor. Change the list of options to buttons rather than divs with onClick attributes. Change how options are created and passed into Combobox component.
Closes the dropdown menu when clicked outside the component.
This allows the dropdown to contain all available children when it is first opened when otherwise it would be empty.
small lintining
Creates a simple span that indicates no matches for a given input value.
This makes it easier to open the menu with Space when you tab into the input, since the text gets highlighted (and would otherwise get deleted/replaced by ' ').
lightened up the grays on focus and hover to leave the writing slightly more legible.
Gazook89
requested review from
5e-Cleric and
ericscheid
and removed request for
ericscheid
| if(this.refs.dropdown && !this.refs.dropdown.contains(e.target)) { | ||
| this.handleDropdown(false); | ||
|
|
||
| document.addEventListener('pointerdown', handleClickOutside); |
There was a problem hiding this comment.
This is also adding a new event listener every time showDropdown changes.
If you want to add a listener once when the component first mounts, change the [showDropdown] to an empty array []
There was a problem hiding this comment.
If just change this to onMount [], it doesn't work. But it works if i remove the showDropdown condition in the handler:
const handleClickOutside = (evt)=>{
console.log(componentRef.current, showDropdown)
if(componentRef.current && !componentRef.current.contains(evt.target)) {
setShowDropdown(false);
}
};
So that's what i'll do.
client/components/combobox.jsx
Outdated
| useEffect(()=>{ | ||
| onSelect(inputValue); | ||
| // handleInputChange({ target: { value: inputValue } }); | ||
| }, [inputValue]); |
There was a problem hiding this comment.
useEffect is not usually not needed for this type of case. Here, we trigger two renders, first when inputValue state changes, and then again when onSelect() changes some state of the parent metaDataEditor.
Instead of setting inputValue and then handling side effects later in useEffect, both setInputValue and calling onSelect() should happen together in a handler function, probably in your handleInputChange() function.
| // Handle keyboard navigation | ||
| const handleKeyDown = (evt)=>{ | ||
| const modifiers = ['Meta', 'Shift', 'Alt', 'Control', 'Tab']; | ||
| if(inputFocused || (currentOption >= 0)) { |
There was a problem hiding this comment.
If we are already focused, why do we need to keep setting focus for the different keys below?
| // handleInputChange({ target: { value: inputValue } }); | ||
| }, [inputValue]); | ||
|
|
||
| useEffect(()=>{ |
There was a problem hiding this comment.
Similar comment here. Changing currentOption and the side effect of focusing the element should happen inside of a handler function, not inside useEffect.
no destructuring.
And removes the dependence on showDropdown of that effect.
|
Thanks for reviewing. I'm going to leave some comments unaddressed because I'm in the middle of refiguring out how focus is managed and a few small behavior changes as part of the "align with WCAG" portion of this. I didn't think I'd get two code reviews so quick :) but they were still valuable and I changed a few things as suggested. |
5e-Cleric
added
cleanup
Description
This is a rework of the Combobox component. It is now a Functional component. It makes a few small changes to structure (the options are all buttons now). It also adds better keyboard navigation (which is pretty much all of the "new lines of code"). Beyond that, it is mostly the same at this point.
Right now, only the Language picker uses this component. I started to rework it because I want to use it in the TagInput component (aka StringArrayEditor).
Beyond whatever is suggested in review, some additional things I want to do:
Then, as a subsequent PR, I want to work out a good way to get submenus to work.
Related Issues or Discussions
QA Instructions, Screenshots, Recordings
Try out the Language picker, using your mouse and navigating it with the keyboard. You should be able to:
Possibly some of these behaviors will change as i look at WCAG for comboboxes and I'll try to update this list.
Reviewer Checklist
*Reviewers, refer to this list when testing features, or suggest new items *
Copy this list