-
Notifications
You must be signed in to change notification settings - Fork 24
Add keyboard navigation to Groups page #236
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
- Added `role="button"`, `tabIndex={0}`, and `aria-label` to group cards
- Implemented `onKeyDown` handler for Enter/Space activation on cards
- Added `aria-label` to search input
- Verified changes with Playwright script (mocked auth and data)
- Updated documentation in `.Jules/` tracking files
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughThe PR adds keyboard navigation and accessibility support to the Groups page. Changes include ARIA labels and keyboard event handlers on group cards, focus styling improvements, expanded accessibility documentation, and updates to changelog and task tracking. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
✅ Deploy Preview for split-but-wiser ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pages/Groups.tsx (1)
168-189: Consider using semantic<button>element instead ofrole="button".The keyboard navigation implementation is correct—
onKeyDownproperly handles Enter/Space withpreventDefault(), and focus styling is theme-aware. However, using a native<button>element (ormotion.button) would be semantically preferable and provide built-in keyboard activation without the manualonKeyDownhandler.Framer Motion supports
motion.button, which should work with your existing animations.♻️ Suggested refactor
- <motion.div + <motion.button + type="button" key={group._id} layout variants={itemVariants} whileHover={{ scale: 1.02, rotate: isNeo ? 1 : 0 }} whileTap={{ scale: 0.98 }} onClick={() => navigate(`/groups/${group._id}`)} - role="button" - tabIndex={0} aria-label={`View details for group ${group.name}`} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - navigate(`/groups/${group._id}`); - } - }} - className={`group cursor-pointer transition-all duration-300 relative overflow-hidden flex flex-col h-full focus:outline-none focus:ring-4 focus:ring-blue-500/50 + className={`group cursor-pointer transition-all duration-300 relative overflow-hidden flex flex-col h-full text-left focus:outline-none focus:ring-4 focus:ring-blue-500/50 ${isNeo ? `bg-white border-2 border-black shadow-[4px_4px_0px_0px_rgba(0,0,0,1)] hover:shadow-[6px_6px_0px_0px_rgba(0,0,0,1)] rounded-none focus:shadow-[6px_6px_0px_0px_rgba(0,0,0,1)]` : `rounded-3xl border shadow-lg backdrop-blur-md ${mode === 'dark' ? 'border-white/20 bg-white/5 hover:bg-white/10' : 'border-black/5 bg-white/60 hover:bg-white/80'}`} `} > {/* ... card content ... */} - </motion.div> + </motion.button>Note: You may need to add
text-leftto preserve text alignment since buttons center content by default.
🤖 Fix all issues with AI agents
In @.Jules/knowledge.md:
- Around line 243-246: Add a blank line before the "### Playwright Verification
Patterns" heading in the .Jules/knowledge.md file to satisfy markdownlint;
locate the "## Testing & Verification" section and ensure there is an empty line
separating it from the "### Playwright Verification Patterns" heading (the line
starting with "### Playwright Verification Patterns") so the heading is preceded
by a single blank line.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.Jules/changelog.md.Jules/knowledge.md.Jules/todo.mdweb/pages/Groups.tsx
🧰 Additional context used
📓 Path-based instructions (1)
web/pages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Web pages should be created as components in
web/pages/and added to routing configuration
Files:
web/pages/Groups.tsx
🪛 Biome (2.1.2)
web/pages/Groups.tsx
[error] 175-175: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
🪛 markdownlint-cli2 (0.18.1)
.Jules/knowledge.md
245-245: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (5)
.Jules/changelog.md (1)
13-13: LGTM!The changelog entry accurately documents the keyboard navigation feature and follows the established format.
web/pages/Groups.tsx (1)
135-145: LGTM!Adding
aria-label="Search groups"improves screen reader experience for the search input..Jules/knowledge.md (1)
107-134: Good accessibility documentation.This pattern documentation is clear and comprehensive. Note that if the Groups page is refactored to use semantic
<button>elements (as suggested in the Groups.tsx review), consider updating this section to reflect the preferred approach—or explicitly note that therole="button"pattern is for cases where semantic buttons aren't feasible..Jules/todo.md (2)
23-28: LGTM!Task correctly marked as completed with accurate metadata matching the implementation.
156-159: LGTM!The completed task entry accurately documents the keyboard navigation implementation with appropriate impact description.
|
@jules web/pages/Groups.tsx (1) 168-189: Consider using semantic element instead of role="button". The keyboard navigation implementation is correct—onKeyDown properly handles Enter/Space with preventDefault(), and focus styling is theme-aware. However, using a native element (or motion.button) would be semantically preferable and provide built-in keyboard activation without the manual onKeyDown handler.Framer Motion supports motion.button, which should work with your existing animations. |
Good suggestion! I've replaced |
- Replaced `motion.div` with `motion.button` for better semantics
- Removed manual `onKeyDown` handler (native button handles Enter/Space)
- Removed explicit `role="button"` and `tabIndex={0}`
- Added `w-full text-left` to maintain layout consistency
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Devasy
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.
LGTM
Implemented full keyboard navigation support for the Groups page in the web application.
Key changes:
PR created automatically by Jules for task 667287562603111129 started by @Devasy23
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.