Add aria-labels to icon-only buttons - #220
Conversation
|
@charlie12520 is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
More reviews will be available in 43 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds ChangesAccessibility improvements across application components
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/mode-toggle.tsx (1)
21-24: ⚡ Quick winConsider removing the redundant sr-only span.
The button now has both
aria-label="Toggle theme"(Line 21) and ansr-onlyspan with the same text (Line 24). Whenaria-labelis present, it takes precedence and thesr-onlycontent is ignored by screen readers. Thesr-onlyspan is now redundant.♻️ Proposed cleanup
- <Button variant="outline" size="icon" aria-label="Toggle theme"> + <Button variant="outline" size="icon" aria-label="Toggle theme"> <Sun className="h-[1.2rem] w-[1.2rem] scale-100 rotate-0 transition-all dark:scale-0 dark:-rotate-90" /> <Moon className="absolute h-[1.2rem] w-[1.2rem] scale-0 rotate-90 transition-all dark:scale-100 dark:rotate-0" /> - <span className="sr-only">Toggle theme</span> </Button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/mode-toggle.tsx` around lines 21 - 24, The Button already provides an accessible name via aria-label="Toggle theme", so remove the redundant visually-hidden label element: delete the <span className="sr-only">Toggle theme</span> in the Mode Toggle component (where Button, Sun, and Moon are rendered) and keep the aria-label on the Button to preserve accessibility.components/ui/sidebar.tsx (1)
270-278: 💤 Low valueMinor casing inconsistency between aria-label and sr-only text.
The aria-label uses lowercase "sidebar" while the sr-only span uses uppercase "Sidebar". Consider aligning them for consistency.
Suggested alignment
- aria-label="Toggle sidebar" + aria-label="Toggle Sidebar"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ui/sidebar.tsx` around lines 270 - 278, Align the casing between the aria-label and the screen-reader text: update the aria-label on the button (currently aria-label="Toggle sidebar") or the sr-only span text (the <span className="sr-only">Toggle Sidebar</span>) so both use the same casing (e.g., "Toggle Sidebar") to ensure consistency; the change should be made where the onClick handler/toggleSidebar call and props are defined in the component so both the aria-label and sr-only span match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@components/mode-toggle.tsx`:
- Around line 21-24: The Button already provides an accessible name via
aria-label="Toggle theme", so remove the redundant visually-hidden label
element: delete the <span className="sr-only">Toggle theme</span> in the Mode
Toggle component (where Button, Sun, and Moon are rendered) and keep the
aria-label on the Button to preserve accessibility.
In `@components/ui/sidebar.tsx`:
- Around line 270-278: Align the casing between the aria-label and the
screen-reader text: update the aria-label on the button (currently
aria-label="Toggle sidebar") or the sr-only span text (the <span
className="sr-only">Toggle Sidebar</span>) so both use the same casing (e.g.,
"Toggle Sidebar") to ensure consistency; the change should be made where the
onClick handler/toggleSidebar call and props are defined in the component so
both the aria-label and sr-only span match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f26ec4e-3180-4f96-890e-d3cbb15b0b3c
📒 Files selected for processing (13)
components/bounty-detail/bounty-detail-sidebar-cta.tsxcomponents/bounty-detail/model4-maintainer-dashboard.tsxcomponents/bounty/forms/milestone-builder.tsxcomponents/bounty/refund-status.tsxcomponents/compliance/document-upload.tsxcomponents/global-navbar.tsxcomponents/mode-toggle.tsxcomponents/ui/carousel.tsxcomponents/ui/dialog.tsxcomponents/ui/sheet.tsxcomponents/ui/sidebar.tsxcomponents/ui/stellar-link.tsxcomponents/wallet/wallet-sheet.tsx
Fixes #216
Summary:
Validation:
pnpm install --frozen-lockfilepnpm lint(passes with one existing warning inlib/server-graphql.ts)next build(ran successfully from the repo pre-push hook; only Stellar env fallback warnings were reported)Summary by CodeRabbit
Release Notes
Accessibility Improvements
Style