Added title and aria-labels to buttons#576
Conversation
|
|
WalkthroughAdded title and aria-label accessibility attributes to many interactive buttons across dialogs, media controls, navbar, and theme toggle. No logic, state, control-flow, or exported API changes. (≈24 words) Changes
Sequence Diagram(s)(omitted — changes are only accessibility attributes on existing UI controls; no new control-flow or interactions introduced) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
frontend/src/components/ui/dialog.tsx (1)
71-75: Remove redundantaria-labelassr-onlytext already provides accessible name.The button already has an
sr-onlyspan with "Close" (Line 75), which serves the same purpose asaria-label. Having both is redundant. Thetitleattribute is sufficient for the hover tooltip.Apply this diff to remove the redundant
aria-label:<DialogPrimitive.Close data-slot="dialog-close" className="ring-offset-background focus:ring-ring data-[state=open]:bg-accent data-[state=open]:text-muted-foreground absolute top-4 right-4 rounded-xs opacity-70 transition-opacity hover:opacity-100 focus:ring-2 focus:ring-offset-2 focus:outline-hidden disabled:pointer-events-none [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4" - title='Close' - aria-label='Close' + title="Close" >Additionally, run
prettier --writeto fix the quote style as indicated by the pipeline failure.frontend/src/components/ThemeToggle.tsx (1)
18-21: Fix capitalization inconsistency and remove redundantaria-label.Two issues:
- Capitalization mismatch:
title='Themes'uses capital T whilearia-label='themes'uses lowercase. These should match.- The button already has
sr-onlytext "Toggle theme" (Line 21), makingaria-labelredundant.Apply this diff:
- <Button variant="outline" size="icon" title='Themes' aria-label='themes'> + <Button variant="outline" size="icon" title="Themes">Run
prettier --writeto fix the quote style as indicated by the pipeline failure.frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
78-82: Remove redundantaria-labelassr-onlytext already provides accessible name.The button already has an
sr-onlyspan with "Face Detection Search" (Line 82), which provides the same accessible name asaria-label. Thetitleattribute is sufficient for the hover tooltip.Apply this diff:
variant="ghost" size="icon" className="h-8 w-8 p-1" - title='Face Detection Search' - aria-label='Face Detection Search' + title="Face Detection Search" >Run
prettier --writeto fix the quote style as indicated by the pipeline failure.frontend/src/components/Media/ZoomControls.tsx (1)
26-57: Use consistent quote style for attributes.The file mixes single quotes (
title='Zoom Out') and double quotes (aria-label="Zoom In"). Use consistent double quotes throughout.Run
prettier --writeas indicated by the pipeline failure to automatically fix quote consistency and other formatting issues.frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
65-70: Fix formatting of button attributes.The button opening tag spans multiple lines in an inconsistent manner. While this may work syntactically, it reduces readability.
Run
prettier --writeto automatically format this correctly, as indicated by the pipeline failure.
76-79: Remove redundantaria-labelassr-onlytext already provides accessible name.The button already has an
sr-onlyspan with "Notifications" (Line 79), making thearia-labelredundant. Thetitleattribute is sufficient for the hover tooltip.Apply this diff:
- <Button variant="ghost" size="icon" className="relative" title='Notifications' aria-label='Notifications'> + <Button variant="ghost" size="icon" className="relative" title="Notifications">Run
prettier --writeto fix the quote style as indicated by the pipeline failure.frontend/src/components/Media/ImageCard.tsx (2)
81-90: Fix inconsistency:aria-labelshould match the dynamictitleor be removed.The
titleattribute dynamically reflects the button state (Line 81), but thearia-labelis static as "Add to Favorites" (Line 82). This creates an inconsistency.Additionally, the button already has an
sr-onlyspan with "Favorite" (Line 90), makingaria-labelredundant.Apply this diff to remove the redundant static
aria-label:onClick={(e) => { e.stopPropagation(); setIsFavorite(!isFavorite); }} - title={isFavorite ? 'Remove from favorites' : 'Add to favorites'} - aria-label='Add to Favorites' + title={isFavorite ? "Remove from favorites" : "Add to favorites"} >If you want both dynamic
titleandaria-label, make them match:- title={isFavorite ? 'Remove from favorites' : 'Add to favorites'} - aria-label='Add to Favorites' + title={isFavorite ? "Remove from favorites" : "Add to favorites"} + aria-label={isFavorite ? "Remove from favorites" : "Add to favorites"}Run
prettier --writeto fix the quote style as indicated by the pipeline failure.
97-101: Remove redundantaria-labelassr-onlytext already provides accessible name.The button already has an
sr-onlyspan with "Share" (Line 101), making thearia-labelredundant. Thetitleattribute is sufficient for the hover tooltip.Apply this diff:
className="rounded-full bg-white/20 text-white hover:!bg-white/40 hover:!text-white" onClick={(e) => e.stopPropagation()} - title='Share' - aria-label='Share' + title="Share" >Run
prettier --writeto fix the quote style as indicated by the pipeline failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/components/Dialog/FaceSearchDialog.tsx(1 hunks)frontend/src/components/Media/ImageCard.tsx(2 hunks)frontend/src/components/Media/MediaView.tsx(1 hunks)frontend/src/components/Media/MediaViewControls.tsx(3 hunks)frontend/src/components/Media/ZoomControls.tsx(1 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(2 hunks)frontend/src/components/ThemeToggle.tsx(1 hunks)frontend/src/components/ui/dialog.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/ThemeToggle.tsx (1)
frontend/src/components/ui/button.tsx (1)
Button(59-59)
🪛 GitHub Actions: PR Check
frontend/src/components/ui/dialog.tsx
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
frontend/src/components/Media/ZoomControls.tsx
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
frontend/src/components/Media/MediaViewControls.tsx
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
frontend/src/components/Media/MediaView.tsx
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
frontend/src/components/Media/ImageCard.tsx
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
frontend/src/components/Navigation/Navbar/Navbar.tsx
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
frontend/src/components/Dialog/FaceSearchDialog.tsx
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
frontend/src/components/ThemeToggle.tsx
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
🔇 Additional comments (2)
frontend/src/components/Media/MediaViewControls.tsx (1)
38-127: LGTM! Run Prettier to fix quote style.The accessibility enhancements look good. The dynamic
titleon the favorites button (Line 89) properly reflects the button's current state, and all other buttons have appropriate static titles.Run
prettier --writeto fix the quote style as indicated by the pipeline failure.frontend/src/components/Media/ZoomControls.tsx (1)
35-40: Fix syntax error:aria-labelis incorrectly placed withinclassNamestring.Line 37 has
aria-label='Reset'incorrectly placed in the middle of theclassNameattribute string. This is a syntax error that will break the component.Apply this diff to fix:
<button onClick={onReset} - className="rounded-md bg-white/10 px-3 py-2 text-sm font-medium text-white transition-all duration-200 hover:bg-white/20 hover:shadow-md" aria-label='Reset' + className="rounded-md bg-white/10 px-3 py-2 text-sm font-medium text-white transition-all duration-200 hover:bg-white/20 hover:shadow-md" + aria-label="Reset" + title="Reset" >Likely an incorrect or invalid review comment.
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/Media/ZoomControls.tsx (1)
35-41: Remove unnecessaryaria-labelfrom text button.The Reset button has visible text content ("Reset"), so the
aria-labelattribute is redundant. According to accessibility best practices,aria-labelshould only be used on icon-only or visually ambiguous buttons. When a button has visible text, the text content automatically serves as the accessible name. Addingaria-labelcan override this and potentially confuse screen reader users.Apply this diff to remove the redundant attribute:
<button onClick={onReset} className="rounded-md bg-white/10 px-3 py-2 text-sm font-medium text-white transition-all duration-200 hover:bg-white/20 hover:shadow-md" - aria-label="Reset" > Reset </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/src/components/Dialog/FaceSearchDialog.tsx(1 hunks)frontend/src/components/Media/ImageCard.tsx(2 hunks)frontend/src/components/Media/MediaViewControls.tsx(3 hunks)frontend/src/components/Media/ZoomControls.tsx(3 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(2 hunks)frontend/src/components/ThemeToggle.tsx(1 hunks)frontend/src/components/ui/dialog.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/components/Dialog/FaceSearchDialog.tsx
- frontend/src/components/Media/ImageCard.tsx
- frontend/src/components/ThemeToggle.tsx
- frontend/src/components/Navigation/Navbar/Navbar.tsx
- frontend/src/components/Media/MediaViewControls.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Backend Tests
🔇 Additional comments (2)
frontend/src/components/Media/ZoomControls.tsx (1)
30-30: LGTM! Title attributes improve discoverability.Adding native hover tooltips to icon-only buttons enhances the user experience for mouse users.
Also applies to: 47-47, 56-56
frontend/src/components/ui/dialog.tsx (1)
68-76: Accessibility attributes correctly applied.The
titleandaria-labeladditions improve both hover discoverability and screen reader support. Whilearia-labeltechnically overrides thesr-onlytext, having both with matching values is harmless and ensures consistent behavior across different assistive technologies.
|
|
|
|
1 similar comment
|
|
|
@Pritom2357 Please resolve the conflicts. |
|
|
1 similar comment
|
|
rahulharpal1603
added
bug
|
@rahulharpal1603 done, please check |
|
|
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
@Pritom2357 Thanks!
Sorry for the delay though.
2 participants
Description
Summary
Why
Changes
Linked Issue
issue: 575
Demo:
Testing
Checklist
Summary by CodeRabbit