-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Notes: Add keyboard support for tree navigation #73136
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
Conversation
|
Size Change: +173 B (+0.01%) Total Size: 2.41 MB
ℹ️ View Unchanged
|
| test.describe( 'Keyboard navigation', () => { | ||
| test( 'should expand or collapse a comment with Enter key', async ( { | ||
| editor, | ||
| const KEY_COMBINATIONS = [ |
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.
Since there was already a test for expanding and collapsing notes using the Enter key, I extended it to also test the left and right arrow keys.
| } | ||
| ); | ||
|
|
||
| test( 'should move to the adjacent comment with arrow keys', async ( { |
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.
The diffs are not showing well, but I simply added the following two new tests.
should move to the adjacent comment with arrow keysshould move to the first or last comment with Home or End keys
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in 1add4c2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19233210516
|
adamsilverstein
left a comment
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.
Tremendous!
* Notes: Add keyboard support for tree navigation * Refactoring * Add e2e tests
|
I just cherry-picked this PR to the wp/6.9 branch to get it included in the next release: 8651bcf |
|
Nice work @t-hamano - this worked well in my testing. |
| setCanvasMinHeight, | ||
| ] ); | ||
|
|
||
| const handleThreadNavigation = useCallback( |
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.
The memoization here is unnecessary and does nothing.
Why?
- You still passing regular callback to
onKeyDown. It will have new reference on each re-render. - The
handleThreadNavigationisn't used as dependency for other hook or passed as prop to memoized component.
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.
Thanks for the feedback, I will submit a pull request so that it can be backported before the RC1 release.
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.
Thanks, @t-hamano!
Closes #72958
What?
This PR adds the following keyboard navigation to improve the accessibility of Note
How?
To navigate to the adjacent note, the first note, or the last note, information about all the notes is required. Therefore, I moved the keydown event from the
Threadcomponent to the upstreamCommentscomponent.Testing Instructions
Screenshots or screencast
b0b971ebd7885f35bbe2e890c7cace85.mp4