fix(docs): resolve keyboard navigation issues on Actions menu #4528
Conversation
…u and preview buttons (frontio-ai#4459)
|
|
@mohammadshahidbeigh is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant User
participant WindowActions
participant KeyboardEvent
User->>WindowActions: Render component
WindowActions-->>User: Display menu
User->>KeyboardEvent: Press Escape key
KeyboardEvent->>WindowActions: Trigger handleKeyDown
WindowActions->>WindowActions: Set isMenuVisible to false
WindowActions-->>User: Hide menu
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/docs/components/code-window/window-actions.tsx (2)
30-36: Prevent event listener interferenceThe global keydown listener might interfere with other modal dialogs or dropdowns. Consider:
- Adding a check if the menu is visible before handling the escape key
- Using React's onKeyDown prop instead of a global listener
-useEffect(() => { - window.addEventListener("keydown", handleKeyDown); - - return () => { - window.removeEventListener("keydown", handleKeyDown); - }; -}, [handleKeyDown]);And update the component's div to handle keyboard events directly:
<div aria-hidden={!isMenuVisible} aria-labelledby="window-actions-title" + onKeyDown={isMenuVisible ? handleKeyDown : undefined} className={clsx(
38-38: Improve visibility transitionsReturning
nullcauses abrupt DOM removal which might be jarring for users. Consider:
- Using CSS transitions for smooth show/hide
- Keeping the element in DOM but visually hidden for better accessibility
-if (!isMenuVisible) return null; +const visibility = isMenuVisible ? "visible" : "hidden";And update the component's className:
className={clsx( "flex items-center sticky top-0 left-0 px-4 z-10 justify-between h-8 bg-code-background w-full", + visibility === "hidden" && "invisible opacity-0", + "transition-opacity duration-200", className, )}
| <div className="flex basis-1/3"> | ||
| <button className="ml-auto text-xs text-white/50" onClick={() => setIsMenuVisible(false)}> | ||
| Close | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve close button accessibility and UX
The close button needs the following improvements:
- Add
aria-labelfor screen readers - Add keyboard shortcut hint using
aria-keyshortcuts - Enhance button styling for better visibility
<div className="flex basis-1/3">
<button
+ aria-label="Close window actions menu"
+ aria-keyshortcuts="Escape"
- className="ml-auto text-xs text-white/50"
+ className="ml-auto text-xs text-white/50 hover:text-white/70 focus:text-white/70 focus:outline-none focus:ring-2 focus:ring-white/20 rounded px-2 py-1"
onClick={() => setIsMenuVisible(false)}
>
Close
</button>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="flex basis-1/3"> | |
| <button className="ml-auto text-xs text-white/50" onClick={() => setIsMenuVisible(false)}> | |
| Close | |
| </button> | |
| </div> | |
| <div className="flex basis-1/3"> | |
| <button | |
| aria-label="Close window actions menu" | |
| aria-keyshortcuts="Escape" | |
| className="ml-auto text-xs text-white/50 hover:text-white/70 focus:text-white/70 focus:outline-none focus:ring-2 focus:ring-white/20 rounded px-2 py-1" | |
| onClick={() => setIsMenuVisible(false)} | |
| > | |
| Close | |
| </button> | |
| </div> |
| aria-hidden={!isMenuVisible} // Visibility state | ||
| aria-labelledby="window-actions-title" | ||
| className={clsx( | ||
| "flex items-center sticky top-0 left-0 px-4 z-10 justify-between h-8 bg-code-background w-full", | ||
| className, | ||
| )} | ||
| role="dialog" // Semantic role |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance ARIA attributes for better screen reader support
Add the following ARIA attributes to improve accessibility:
aria-modal="true"to indicate modal behavioraria-describedbyfor additional contexttabindex="-1"to manage focus properly
<div
aria-hidden={!isMenuVisible}
aria-labelledby="window-actions-title"
+ aria-modal="true"
+ aria-describedby="window-actions-description"
+ tabIndex={-1}
role="dialog"
className={clsx(Committable suggestion skipped: line range outside the PR's diff.
| const [isMenuVisible, setIsMenuVisible] = useState(true); | ||
|
|
||
| const handleKeyDown = useCallback((e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { | ||
| setIsMenuVisible(false); // Close the menu | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance focus management and initial state handling
Consider the following improvements:
- Initialize
isMenuVisibletofalseby default to prevent the menu from being open on mount - Implement focus management to return focus to the trigger element when menu closes
- Add a focus trap to keep focus within the menu while it's open
-const [isMenuVisible, setIsMenuVisible] = useState(true);
+const [isMenuVisible, setIsMenuVisible] = useState(false);
+const previousFocusRef = useRef<HTMLElement | null>(null);
+
+const handleClose = useCallback(() => {
+ setIsMenuVisible(false);
+ if (previousFocusRef.current) {
+ previousFocusRef.current.focus();
+ }
+}, []);
const handleKeyDown = useCallback((e: KeyboardEvent) => {
if (e.key === "Escape") {
- setIsMenuVisible(false);
+ handleClose();
}
}, []);Committable suggestion skipped: line range outside the PR's diff.
wingkwong
changed the title
📝 Description
This pull request addresses accessibility issues related to keyboard navigation in the Actions menu and preview buttons. It implements functionality to close the Actions menu using the
Escapekey and improves focus management.⛳️ Current behavior (updates)
Escapekey.🚀 New behavior
Escapekey.💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
This update enhances the accessibility of the NextUI components, making them more user-friendly for keyboard and screen reader users. It resolves issue #4459.
Summary by CodeRabbit
New Features
Accessibility