feat: add combo box component #399
Conversation
|
There is a bug where the option list displays as empty if there is already a value selected. I think it stems from the filtering logic. |
There was a problem hiding this comment.
Its great to see progress on this!
One overarching comment -I think now we need to see if we can simplify the component state more. Can we track less things in the state, have less cases where we need to manually call things in a certain order (update the selected value, update the focus, update the focus mode, update whats open)?
Update: @jim and I paired on this and started looking at if its possible to separate out behaviors the input takes from the dropdown list (instead of having ComboBox manage the state for everything, delegate some of that to nested components).
| onBlur={handleInputBlur} | ||
| onClick={handleInputClick} | ||
| onKeyDown={handleInputKeyDown} | ||
| onChange={(e): void => setInputValue(e.target.value)} |
There was a problem hiding this comment.
I think we want to allow the consumer to still be able to call their own onChange passed in from the selectProps.onChange. I can't tell would that still happen the way things are written now? Lets write a test case to make sure.
| return { | ||
| ...state, | ||
| inputValue: '', | ||
| selectedOption: undefined, |
There was a problem hiding this comment.
Also need to reset filteredOptions on this action to props.action
There was a problem hiding this comment.
I added a test stub to cover this case.
| } = props | ||
|
|
||
| let defaultLabel = '' | ||
| if (defaultValue) { |
There was a problem hiding this comment.
This is a bit confusing because the defaultValue is usually the display text.
Right now its written as if that string will be the option id. Maybe we should change to selectedOption and just pass the whole option object in? Or else rename this defaultValue to something like defaultOptionId
|
@jim Checking in! Thanks for your work so far on this. What is the outstanding work here? Would love to get this merged and can help make sure that happens. |
|
@haworku There are some missing tests to implement, and probably a few to write. The biggest TODO, though, is to make a decision about how to proceed in terms of what props are required to use the ComboBox. Right now, we expect a |
haworku
requested review from
ahobson,
gidjin,
sarboc and
tinyels
as code owners
Add Combo Box
This PR is a first pass at adding the combo box component. We are merging to a feature branch for now since we anticipate refactoring the component.
The initial implement uses a prop for handling the options list and
setFieldValueto control the input value.There are significant tests added, which will help be a guard us as future refactors are made.
Related Issues or PRs
related to #170
How To Test