feat: add tooltips to navbar elements #652
Conversation
|
|
WalkthroughAdds Tooltip wrappers and small accessibility/text tweaks to interactive controls in Navbar, ThemeToggle, and FaceSearchDialog, providing hover labels for close/clear, search, face-search, theme-toggle, and profile/settings elements; no public API or control-flow changes. Changes
Sequence Diagram(s)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)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Navigation/Navbar/Navbar.tsx(4 hunks)frontend/src/components/ThemeToggle.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/ThemeToggle.tsx (4)
frontend/src/contexts/ThemeContext.tsx (1)
useTheme(66-73)frontend/src/components/ui/tooltip.tsx (3)
Tooltip(59-59)TooltipTrigger(59-59)TooltipContent(59-59)frontend/src/components/ui/dropdown-menu.tsx (4)
DropdownMenu(240-240)DropdownMenuTrigger(242-242)DropdownMenuContent(243-243)DropdownMenuItem(246-246)frontend/src/components/ui/button.tsx (1)
Button(59-59)
frontend/src/components/Navigation/Navbar/Navbar.tsx (3)
frontend/src/components/ui/tooltip.tsx (3)
Tooltip(59-59)TooltipTrigger(59-59)TooltipContent(59-59)frontend/src/features/searchSlice.ts (1)
clearSearch(21-24)frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
FaceSearchDialog(25-153)
🔇 Additional comments (3)
frontend/src/components/ThemeToggle.tsx (2)
10-14: LGTM!The Tooltip component imports are correct and follow the established import pattern.
21-61: Verify tooltip behavior with nested dropdown interaction.The tooltip wrapping the dropdown trigger should work correctly with
asChild, but please confirm:
- Hovering shows the tooltip without immediately opening the dropdown
- Clicking still opens the dropdown menu properly
- Keyboard navigation (Tab, Enter, Escape) works as expected
- The tooltip dismisses appropriately when the dropdown opens
Minor note: Line 33's
sr-onlyspan is redundant with thearia-labelon line 29—both provide the same screen reader text. Consider removing thesr-onlyspan.frontend/src/components/Navigation/Navbar/Navbar.tsx (1)
9-13: LGTM!The Tooltip component imports are correct.
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| onClick={() => dispatch(clearSearch())} | ||
| className="absolute -top-1 -right-1 flex h-3 w-3 items-center justify-center rounded-full bg-red-600 text-[10px] leading-none text-white" | ||
| aria-label="Close" | ||
| > | ||
| ✕ | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p>Clear search</p> | ||
| </TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
Align aria-label with tooltip text for consistency.
The button's aria-label="Close" (line 55) doesn't match the tooltip text "Clear search" (line 61). Since the action clears the search query, "Clear search" is more descriptive. Update the aria-label to match.
Apply this diff:
- aria-label="Close"
+ aria-label="Clear search"📝 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.
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <button | |
| onClick={() => dispatch(clearSearch())} | |
| className="absolute -top-1 -right-1 flex h-3 w-3 items-center justify-center rounded-full bg-red-600 text-[10px] leading-none text-white" | |
| aria-label="Close" | |
| > | |
| ✕ | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent> | |
| <p>Clear search</p> | |
| </TooltipContent> | |
| </Tooltip> | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <button | |
| onClick={() => dispatch(clearSearch())} | |
| className="absolute -top-1 -right-1 flex h-3 w-3 items-center justify-center rounded-full bg-red-600 text-[10px] leading-none text-white" | |
| aria-label="Clear search" | |
| > | |
| ✕ | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent> | |
| <p>Clear search</p> | |
| </TooltipContent> | |
| </Tooltip> |
🤖 Prompt for AI Agents
In frontend/src/components/Navigation/Navbar/Navbar.tsx around lines 50 to 63,
the button's aria-label is "Close" while the tooltip reads "Clear search";
update the button's aria-label to "Clear search" so it matches the tooltip and
accurately describes the action (i.e., replace aria-label="Close" with
aria-label="Clear search").
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <div> | ||
| <FaceSearchDialog /> | ||
| </div> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p>Search by face</p> | ||
| </TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
Remove the intermediate div wrapper—it prevents proper tooltip interaction.
The TooltipTrigger asChild expects its direct child to be an interactive element that can accept merged props. Wrapping FaceSearchDialog in a <div> (line 78) means the tooltip attaches to a non-interactive element, which can break hover behavior and accessibility.
Recommended approach: Remove the div wrapper and apply TooltipTrigger asChild directly to the trigger element. Since FaceSearchDialog renders a Dialog with a DialogTrigger > Button, you'll need to restructure slightly. One solution:
- <Tooltip>
- <TooltipTrigger asChild>
- <div>
- <FaceSearchDialog />
- </div>
- </TooltipTrigger>
- <TooltipContent>
- <p>Search by face</p>
- </TooltipContent>
- </Tooltip>
+ <FaceSearchDialog />Then modify FaceSearchDialog.tsx to wrap its DialogTrigger button with Tooltip internally, similar to how you've done with the search button and avatar in this file.
Minor note: The tooltip says "Search by face" but FaceSearchDialog's button has aria-label="Face Detection Search". Align the terminology for consistency.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/Navigation/Navbar/Navbar.tsx around lines 76 to 85,
remove the intermediate <div> used as the child of <TooltipTrigger asChild> so
the TooltipTrigger can attach directly to the interactive trigger; either render
<TooltipTrigger asChild><FaceSearchDialog /></TooltipTrigger> (if
FaceSearchDialog forwards the trigger props) or move the Tooltip wrapping into
FaceSearchDialog so its DialogTrigger button is the direct child of the
TooltipTrigger; update FaceSearchDialog to forward refs/props or to wrap its
DialogTrigger button with Tooltip internally, and align the tooltip text "Search
by face" with the button aria-label (e.g., change to "Face Search" or "Face
Detection Search") for consistent accessibility labeling.
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | ||
| aria-label="Search" | ||
| > | ||
| <Search className="h-4 w-4" /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p>Search images</p> | ||
| </TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
Align aria-label with tooltip text for consistency.
The button's aria-label="Search" (line 92) is less specific than the tooltip text "Search images" (line 98). Update the aria-label to match.
Apply this diff:
- aria-label="Search"
+ aria-label="Search images"📝 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.
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <button | |
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | |
| aria-label="Search" | |
| > | |
| <Search className="h-4 w-4" /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent> | |
| <p>Search images</p> | |
| </TooltipContent> | |
| </Tooltip> | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <button | |
| className="text-muted-foreground hover:bg-accent dark:hover:bg-accent/50 hover:text-foreground mx-1 cursor-pointer rounded-sm p-2" | |
| aria-label="Search images" | |
| > | |
| <Search className="h-4 w-4" /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent> | |
| <p>Search images</p> | |
| </TooltipContent> | |
| </Tooltip> |
🤖 Prompt for AI Agents
frontend/src/components/Navigation/Navbar/Navbar.tsx around lines 88 to 100: the
button's aria-label currently reads "Search" while the TooltipContent says
"Search images"; update the button's aria-label to exactly "Search images" so
screen readers match the visible tooltip and improve accessibility/consistency.
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <a href="/settings" className="p-2"> | ||
| <img | ||
| src={userAvatar || '/photo1.png'} | ||
| className="hover:ring-primary/50 h-8 w-8 cursor-pointer rounded-full transition-all hover:ring-2" | ||
| alt="User avatar" | ||
| /> | ||
| </a> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p>Profile settings</p> | ||
| </TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add aria-label to the profile settings link.
The anchor on line 113 is missing an aria-label, which would improve accessibility for screen reader users. While the image has alt="User avatar", the link's purpose (navigating to settings) isn't explicitly conveyed.
Apply this diff:
- <a href="/settings" className="p-2">
+ <a href="/settings" className="p-2" aria-label="Profile settings">📝 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.
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <a href="/settings" className="p-2"> | |
| <img | |
| src={userAvatar || '/photo1.png'} | |
| className="hover:ring-primary/50 h-8 w-8 cursor-pointer rounded-full transition-all hover:ring-2" | |
| alt="User avatar" | |
| /> | |
| </a> | |
| </TooltipTrigger> | |
| <TooltipContent> | |
| <p>Profile settings</p> | |
| </TooltipContent> | |
| </Tooltip> | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <a href="/settings" className="p-2" aria-label="Profile settings"> | |
| <img | |
| src={userAvatar || '/photo1.png'} | |
| className="hover:ring-primary/50 h-8 w-8 cursor-pointer rounded-full transition-all hover:ring-2" | |
| alt="User avatar" | |
| /> | |
| </a> | |
| </TooltipTrigger> | |
| <TooltipContent> | |
| <p>Profile settings</p> | |
| </TooltipContent> | |
| </Tooltip> |
🤖 Prompt for AI Agents
In frontend/src/components/Navigation/Navbar/Navbar.tsx around lines 111 to 124,
the anchor linking to /settings is missing an aria-label which means screen
readers don't get the explicit purpose of the link; add a descriptive aria-label
attribute to the <a> (for example aria-label="Open profile settings" or similar
localized text) while keeping the existing image alt intact so the link's
function is conveyed to assistive technologies.
|
|
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/src/components/Navigation/Navbar/Navbar.tsx (3)
50-63: Update aria-label to match tooltip text.The button's
aria-label="Close"(line 55) doesn't align with the tooltip text "Clear search" (line 61). For consistency and better accessibility, the aria-label should match the tooltip content.Apply this diff:
- aria-label="Close" + aria-label="Clear search"
78-91: Update aria-label to match tooltip text.The button's
aria-label="Search"(line 83) is less descriptive than the tooltip text "Search images" (line 89). Aligning these improves accessibility and consistency.Apply this diff:
- aria-label="Search" + aria-label="Search images"
102-115: Add aria-label to the profile settings link.The anchor element (line 104) is missing an
aria-labelattribute, which would improve accessibility for screen reader users. While the image hasalt="User avatar", the link's purpose (navigating to settings) isn't explicitly conveyed.Apply this diff:
- <a href="/settings" className="p-2"> + <a href="/settings" className="p-2" aria-label="Profile settings">
🧹 Nitpick comments (1)
frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
107-110: Remove redundant accessibility attribute.The button has both
aria-labeland asr-onlyspan containing the same text. Thearia-labeltakes precedence and makes thesr-onlyspan unnecessary.Apply this diff to remove the redundant
sr-onlyspan:aria-label="Search by face" > <ScanFace className="h-4 w-4" /> - <span className="sr-only">Search by face</span> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Dialog/FaceSearchDialog.tsx(4 hunks)frontend/src/components/Navigation/Navbar/Navbar.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
frontend/src/components/ui/tooltip.tsx (3)
Tooltip(59-59)TooltipTrigger(59-59)TooltipContent(59-59)frontend/src/features/searchSlice.ts (1)
clearSearch(21-24)
frontend/src/components/Dialog/FaceSearchDialog.tsx (2)
frontend/src/components/ui/tooltip.tsx (1)
Tooltip(59-59)frontend/src/components/ui/dialog.tsx (4)
DialogTrigger(142-142)DialogContent(135-135)DialogHeader(138-138)DialogTitle(141-141)
🔇 Additional comments (5)
frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
9-13: LGTM!The tooltip component imports are correctly structured and necessary for the tooltip functionality added throughout this file.
9-13: The review comment is incorrect and can be dismissed.The
Tooltipcomponent infrontend/src/components/ui/tooltip.tsxalready wraps its children with<TooltipProvider>internally (lines 23-25). This means eachTooltipinstance is self-contained and automatically includes the provider—no app-root level provider is required. The Navbar component's usage is correct as-is.Likely an incorrect or invalid review comment.
frontend/src/components/Dialog/FaceSearchDialog.tsx (3)
12-16: LGTM!The Tooltip component imports are correctly added and necessary for the tooltip functionality implemented below.
51-51: LGTM!Good catch replacing the curly quote with a standard ASCII quote for consistency.
121-121: LGTM!The updated dialog title "Search by Face" is more concise and aligns well with the tooltip and accessibility labels used throughout the component.
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
106-109: Remove redundant sr-only span.The
aria-labelon line 106 provides the accessible name and takes precedence over inner content. The<span className="sr-only">on line 109 serves no purpose and can be removed.Apply this diff:
className="h-8 w-8 cursor-pointer p-1" aria-label="Search by face" > <ScanFace className="h-4 w-4" /> - <span className="sr-only">Search by face</span> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Dialog/FaceSearchDialog.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Dialog/FaceSearchDialog.tsx (2)
frontend/src/components/ui/tooltip.tsx (3)
Tooltip(59-59)TooltipTrigger(59-59)TooltipContent(59-59)frontend/src/components/ui/dialog.tsx (4)
DialogTrigger(142-142)DialogContent(135-135)DialogHeader(138-138)DialogTitle(141-141)
🔇 Additional comments (4)
frontend/src/components/Dialog/FaceSearchDialog.tsx (4)
12-16: LGTM!The Tooltip component imports are correctly added and necessary for the new tooltip functionality.
51-51: LGTM!Good correction replacing the curly apostrophe with a straight apostrophe for better ASCII compatibility in the error message.
120-120: LGTM!Excellent consistency improvement. The dialog title now matches the button label and tooltip text, providing a clearer and more cohesive user experience.
99-116: Based on my verification using the codebase analysis and official Radix UI documentation, I can now provide the final response.The original review comment's concerns are not valid.
The code implementation is correct. The correct pattern is to place the outermost wrapper (like Tooltip) outside, and the final DOM element (like Button) inside both DialogTrigger and TooltipTrigger, using asChild. Your code at lines 99-116 follows exactly this pattern:
<Tooltip> <TooltipTrigger asChild> <DialogTrigger asChild> <Button>Regarding TooltipProvider: Tooltip.Provider is now required, and from the codebase inspection, the
Tooltipcomponent infrontend/src/components/ui/tooltip.tsx(lines 23-25) already wraps itself withTooltipProviderinternally. Additionally, the Sidebar component (which likely contains this dialog) wraps content with an explicitTooltipProvider.The tooltip and dialog will compose correctly together without conflicts.
1 participant
Description
Adds tooltips to navbar elements (search, face search, profile, theme toggle) to improve user experience and provide better context for interactive elements.
Changes
Testing
Screen.Recording.2025-11-19.at.4.03.37.PM.mov
Summary by CodeRabbit
UI/UX Enhancements
Accessibility Improvements