A11y/combobox nav refactor and expose list focus and keyboard navigation hook #636
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
April 14, 2022 12:37
hadasfa
changed the title
hadasfa
commented
Apr 20, 2022
| @@ -12,7 +12,7 @@ exports[`BreadcrumbsItem renders correctly when disabled item 1`] = ` | |||
| <span | |||
| aria-disabled="true" | |||
| className="breadcrumb-content disabled" | |||
| tabIndex="0" | |||
| tabIndex="-1" | |||
There was a problem hiding this comment.
not relevant. for fixing tests.
hadasfa
commented
Apr 20, 2022
added 3 commits
April 20, 2022 13:48
…to a11y/combobox-nav � Conflicts: � src/components/Combobox/Combobox.jsx � src/components/Combobox/ComboboxHelpers/ComboboxHelpers.jsx � src/components/Combobox/components/ComboboxItems/ComboboxItems.jsx � src/components/Combobox/components/ComboboxOption/ComboboxOption.jsx
laviomri
reviewed
Apr 20, 2022
| if (!onItemClick || !hasValidIndex || !isItemSelectable(itemIndex)) return; | ||
| if (visualFocusItemIndex !== itemIndex) setVisualFocusItemIndex(itemIndex); | ||
| onItemClick(event, itemIndex); | ||
| triggerByKeyboard.current = event instanceof MouseEvent; |
src/hooks/useActiveDescendantListFocus/__tests__/useActiveDescendantListFocus.interactions.js
Outdated
Show resolved
Hide resolved
src/hooks/useActiveDescendantListFocus/__tests__/useActiveDescendantListFocus.interactions.js
Show resolved
Hide resolved
| <UnstyledList> | ||
| <UnstyledListItem>The component displays a list of values shown in one dimension - horizontal or vertical.</UnstyledListItem> | ||
| <UnstyledListItem>The user can interact with the component items, and therefore, the component is focusable.</UnstyledListItem> | ||
| <UnstyledListItem>When the user focuses on the component, the browser's real focus will always be on an element that is not one of the component's items. Most of the time, the focus will be on the component's Search item or different Text input item.</UnstyledListItem> |
There was a problem hiding this comment.
This is great, I think this single line represents what's the purpose of this hook in the best way
| ); | ||
| }; | ||
|
|
||
| UseActiveDescendantListFocus.propTypes = { |
There was a problem hiding this comment.
Is there any way to put the propTypes on the hook, and import them here to the story or something?
It feels like this file is "not important" (compared to the hook's main file), but it hides within it some valualble info about the hook itself
There was a problem hiding this comment.
We can think about standards regarding this subject but it will look weird
src/hooks/useActiveDescendantListFocus/__tests__/useActiveDescendantListFocus.jest.js
Outdated
Show resolved
Hide resolved
laviomri
approved these changes
Apr 25, 2022
| ); | ||
| } | ||
|
|
||
| function runListUnitTest(isHorizontal) { |
There was a problem hiding this comment.
consider moving the code inside the describe. maybe it's just me, but I found it confusing reading it before reading describe :)
added 2 commits
April 27, 2022 17:36
…to a11y/combobox-nav
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I did a refactor on the combobox keyboard navigation hook for more clear API and for fixing a11y issues inside the previous implementation.
I add a doc which explain when to use this hook and change the combobox code for using the new hook.
also add very basic happy flow interaction tests for the hook.
I would really like to check the on click functionality but I have problems with it. will be happy to explain.