Keyboard nav: nav menu number shortcuts #12226
Conversation
wackerow
requested review from
corwintines,
pettinarip,
minimalsm and
nhsz
as code owners
github-actions
bot
added
review needed 👀
tooling 🔧
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe recent update enhances the navigation experience in a web application by introducing keyboard navigation capabilities and refining constants related to navigation styling. It specifically adds the ability to focus on navigation sections through number keys, incorporates a new constant for the main navigation ID, and adjusts existing constants for better organization and styling consistency. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/components/Nav/Menu/index.tsx (2 hunks)
- src/components/Nav/Menu/useNavMenu.ts (2 hunks)
- src/lib/constants.ts (2 hunks)
Additional comments: 2
src/lib/constants.ts (1)
- 79-83: The introduction of
MAIN_NAV_IDand adjustments to other constants likeNAV_BAR_PX_HEIGHTandNAV_PYare well-aligned with the PR's objectives to enhance navigation accessibility. A few considerations:
Consistency: Ensure that the new and adjusted constants are consistently used across the application. This includes checking that
MAIN_NAV_IDis correctly applied wherever the main navigation is referenced.Documentation: While the constants are self-explanatory to an extent, adding comments to describe their purpose, especially for the newly introduced ones, can improve maintainability and understanding for future contributors.
Impact on Styling: The movement and duplication of
NAV_PYmight have implications on the styling consistency across the site. Verify that this change does not introduce unintended styling discrepancies.Consider adding comments for new constants and verifying the consistent use and impact of these changes across the application.
src/components/Nav/Menu/index.tsx (1)
- 38-43: The integration of
MAIN_NAV_IDinto theMenucomponent by assigning it to theidprop ofUnorderedListis a crucial step towards enabling keyboard navigation. This change is straightforward and aligns with the PR's objectives. However, consider the following:
Accessibility: Ensure that the use of
idfor keyboard navigation does not inadvertently affect the accessibility of the navigation menu. It's important that the navigation structure remains semantically correct and accessible to screen readers and other assistive technologies.Styling Implications: Verify that the addition of the
idattribute does not conflict with existing CSS selectors or JavaScript logic that might be targeting theUnorderedListbased on its attributes.Ensure that the addition of
MAIN_NAV_IDmaintains the accessibility and does not introduce styling or functional conflicts.
| useEventListener("keydown", (event) => { | ||
| if (!document || !event.key.match(/[1-9]/)) return | ||
| if (event.target instanceof HTMLInputElement) return | ||
| if (event.target instanceof HTMLTextAreaElement) return | ||
| if (event.target instanceof HTMLSelectElement) return | ||
|
|
||
| const sectionIdx = parseInt(event.key) - 1 | ||
| if (sectionIdx >= SECTION_LABELS.length) return | ||
|
|
||
| const button = document.querySelector( | ||
| `#${MAIN_NAV_ID} li:nth-of-type(${sectionIdx + 1}) button` | ||
| ) | ||
| if (!button) return | ||
|
|
||
| event.preventDefault() | ||
| ;(button as HTMLButtonElement).focus() | ||
| }) |
There was a problem hiding this comment.
The implementation of keyboard navigation within useNavMenu is well thought out, ensuring that shortcuts do not interfere with text entry in form elements. However, there are a few areas for improvement and clarification:
-
Performance Consideration: Every keydown event triggers checks against the document and event target, which might not be necessary after the initial setup. Consider optimizing this by setting up conditions or states that minimize the number of checks needed after the hook is initialized.
-
Extensibility: The current implementation is tightly coupled with the assumption that there are exactly or less than
SECTION_LABELS.lengthsections. If the navigation were to expand, this logic might need revisiting. Consider making the logic more dynamic to accommodate any number of sections without requiring adjustments. -
Accessibility: While the focus management is a great step towards improving accessibility, ensure that there is also proper ARIA attribute management to communicate the state and role of the navigation elements to assistive technologies.
-
Error Handling: The current implementation silently fails (returns) if conditions are not met. While this is acceptable for control flow, consider logging unexpected states during development for easier debugging.
Consider these improvements to enhance performance, extensibility, and accessibility.
| const sectionIdx = parseInt(event.key) - 1 | ||
| if (sectionIdx >= SECTION_LABELS.length) return | ||
|
|
||
| const button = document.querySelector( |
There was a problem hiding this comment.
One observation: this is not a very react way to do this. We already know all the buttons that are in the menu and we could easily have the reference of each one. This would make all the checking you have to do with this approach a lot easier.
Not a blocker as that works fine too. Just to see what you think, maybe I'm missing something important.
There was a problem hiding this comment.
Yes, I agree with the overall sentiment... I can look at this again, but I did try this is the initial approach... I forget exactly why at the moment, but I hit some issues passing the refs around and getting the correct ones to trigger correctly... I recall it ended up having a lot more code than this, since this approach just takes advantage of the nth-of-type selector. Happy to check this out again, but wanted to just drop a reply.
Description
textarea,inputorselectSummary by CodeRabbit