Refactor InputDropdown and Dropdown #60
Conversation
| } | ||
| }, [focusStatus]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
can this useEffect be group with the useEffect above?
There was a problem hiding this comment.
the event listener needs to be added whenever the section is active, not just when it first becomes active (like the first useEffect). I tried combining them, but then only the first keydown event is registered
There was a problem hiding this comment.
you are cloning the element each time when section is set active right? So wouldn't it be first render of the newly clone section be enough to setup the listener?
| function onLeaveSectionFocus(focusNext: boolean) { | ||
| if (focusedSectionIndex === undefined && focusNext) { | ||
| dispatch({ type: 'FocusSection', newIndex: 0 }); | ||
| } else if (focusedSectionIndex !== undefined && focusedSectionIndex < numSections - 1) { |
There was a problem hiding this comment.
I think focusedSectionIndex !== undefined can be removed
There was a problem hiding this comment.
I restructured it, so now I think the focusedSectionIndex !== undefined should stay
| } else if (focusedSectionIndex !== undefined && focusedSectionIndex < numSections - 1) { | ||
| let newSectionIndex: number | undefined = focusNext ? focusedSectionIndex + 1 : focusedSectionIndex - 1; | ||
| if (newSectionIndex < 0) { | ||
| newSectionIndex = undefined; | ||
| onInputChange(latestUserInput); | ||
| } | ||
| dispatch({ type: 'FocusSection', newIndex: newSectionIndex }); | ||
| } else if (focusedSectionIndex === numSections - 1 && !focusNext) { | ||
| let newSectionIndex: number | undefined = focusedSectionIndex - 1; | ||
| if (newSectionIndex < 0) { | ||
| newSectionIndex = undefined; | ||
| } | ||
| dispatch({ type: 'FocusSection', newIndex: newSectionIndex }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This seems a bit difficult to follow, can we refactor this top level else if to something like this?
const moveToNextSection = focusNext and NOT last section;
const moveToPreviousSection = !focusNext and NOT first section;
else if it's moveToNextSection OR movePreviousSection
update input + move to new section
else if it's (!focusNext and first section) //want to move up to search bar
update to latest input + section index as undefined
There was a problem hiding this comment.
I restructured this a little. Does it make more sense now?
Introduces a DropdownSection component that handles the logic of navigating the focus between options within that section of the dropdown. InputDropdown now only handles navigation between which section is focused and it no longer controls how the dropdown is structured.
J=SLAP-1722
TEST=manual
Check that the search bar in the sample-app behaves as expected