Added useGridKeyboardNavigation #461
Conversation
|
|
||
| const NOOP = () => {}; | ||
|
|
||
| export default function useFullKeyboardListeners({ |
There was a problem hiding this comment.
I saw that we had various places that add similar key listeners, but some had slightly different key-handling. For example, treating only "Enter" as selection key, and not "Space".
If you're ok with it, I can replace some of the existing hooks to use this hook, to reduce code duplication and having a more unified behavior.
src/hooks/useGridKeyboardNavigation/__stories__/useGridKeyboardNavigation.stories.mdx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,106 @@ | |||
| import { useCallback, useLayoutEffect, useState, useContext } from "react"; | |||
| import useFullKeyboardListeners from "../useFullKeyboardListeners"; | |||
| // import { GridKeyboardNavigationContext } from "../../components/GridKeyboardNavigation/GridKeyboardNavigationContext"; | |||
There was a problem hiding this comment.
anything related to the context will be added in the following PR, so it's currently commented out
src/hooks/useGridKeyboardNavigation/__stories__/useGridKeyboardNavigation.stories.mdx
Outdated
Show resolved
Hide resolved
src/hooks/useGridKeyboardNavigation/__stories__/useGridKeyboardNavigation.stories.scss
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,310 @@ | |||
| import { NAV_DIRECTIONS } from "../../useFullKeyboardListeners"; | |||
src/hooks/useGridKeyboardNavigation/__tests__/useGridKeyboardNavigation.jest.js
Outdated
Show resolved
Hide resolved
| it("should set the active index to 0 when focusing for the first time", () => { | ||
| const { result } = renderHookForTest({ }); | ||
|
|
||
| act(() => element.dispatchEvent(new Event('focus'))); |
There was a problem hiding this comment.
can we use "@testing-library/user-event" for the events?.
There was a problem hiding this comment.
I don't think it's possible. the closest thing is userEvent.tab, but it's not useful here.
(the CustomEvent is also irrelevant for userEvent)
There was a problem hiding this comment.
Don't forget that if you need real focus and not only trigger you need element.focus()
"Dispatching the event will not set the focus, that will just trigger the attached event handlers."
There was a problem hiding this comment.
that's a good idea. I tried it though and the event handlers weren't triggered during the test. I suspect that this happens because of the testing environment (on the browser it works).
In any case, I only need to test the handlers, so I'm fine with it as is :)
| */ | ||
|
|
||
| /** | ||
| * A hook which is used for accessible keyboard navigation. Useful for components rendering a list of items that can be navigated and selected with a keyboard. |
There was a problem hiding this comment.
mind blown! @laviomri
love JSDocs!
There was a problem hiding this comment.
glad you like it!
if only there was a way to sync between the JSDoc and the documentation in the stories (like the Returns/Arguments sections), it would have been awesome
src/hooks/useGridKeyboardNavigation/useGridKeyboardNavigation.js
Outdated
Show resolved
Hide resolved
| numberOfItemsInLine, | ||
| itemsCount, | ||
| getItemByIndex, | ||
| onItemClicked: ON_CLICK, |
There was a problem hiding this comment.
When developer will copy this code ON_CLICK will not be defined for him.
Form what I understood from @hadasfa presentations story should work just after copy/paste.
Maybe we need to consider to change to something like:
| onItemClicked: ON_CLICK, | |
| onItemClicked: () = > {console.log("item selected")}, |
There was a problem hiding this comment.
that's a good point. the thing is that the ON_CLICK actually has some functionality (it adds an action to the "Actions" tab of the story). I also assume that the developer will have to change the callback either way (I guess they won't like a console log as a callback 😃 ).
@hadasfa what do you think about this?
There was a problem hiding this comment.
I'm gonna merge it as is for now, but if needed I'd be happy to change it in the future :)
…to feature/omri/user-grid-keyboard-navigation
Task
Added hook for using keyboard navigation in a grid-like layout. Useful for components rendering a list of items that can be navigated and selected with a keyboard.
In the future, we can add support for page up / down, home/end, tab etc...
The next PR contains a context which allows navigation between multiple sections.
(I've had the chance to sync with @MosheZemah about the implementation mid-way).
Basic
npm run plop) to create a new component.PropTypes.Style
Styles are added toIrrelevantNewComponent.modules.scssfile inside of theNewComponentfolder.Component uses CSS Modules.IrrelevantStorybook
/src/NewComponent/__stories__/NewComponent.stories.jsfile.Tests