-
Notifications
You must be signed in to change notification settings - Fork 292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added useGridKeyboardNavigation #461
Conversation
|
||
const NOOP = () => {}; | ||
|
||
export default function useFullKeyboardListeners({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A M A Z I N G!!!!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use "@testing-library/user-event"
for the events?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind blown! @laviomri
love JSDocs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.scss
file inside of theNewComponent
folder.Component uses CSS Modules.IrrelevantStorybook
/src/NewComponent/__stories__/NewComponent.stories.js
file.Tests