🎨 Palette: Enhance accessibility and UX with ARIA labels and Tooltips#527
🎨 Palette: Enhance accessibility and UX with ARIA labels and Tooltips#527ngoiyaeric wants to merge 1 commit intomainfrom
Conversation
- Added `TooltipProvider` to enable tooltips globally. - Implemented `Tooltip` and `aria-label` for icon-only buttons in `Header` and `History`. - Added Tooltips and `aria-label` to desktop `ChatPanel` buttons. - Fixed non-functional mobile search button by adding the missing portal container. - Ensured consistent accessibility text across main navigation and action buttons. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
WalkthroughThis pull request enhances UI accessibility and user feedback by introducing Tooltip components throughout the application. Changes include wrapping the layout with TooltipProvider, adding Tooltip-wrapped buttons in header and chat components, adding ARIA labels for screen reader support, and adjusting mobile navigation with a search portal container. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
A few accessibility/UX consistency issues remain: global TooltipProvider behavior is implicit (defaults may be undesirable), some icon-only actions still lack tooltips (clear attachment), and HeaderSearchButton mixes title with the new tooltip standard. Most importantly, mobile-icons-bar still nests a <Button> inside an <a>, which is an accessibility correctness issue and should be fixed in this PR.
Additional notes (4)
-
Readability |
components/chat-panel.tsx:311-311
Theclear-attachment-buttonis still icon-only and now has anaria-label, but it lacks a tooltip (unlike the other desktop icon-only actions in this file). This creates inconsistent visual affordance within the same panel (some icons explain themselves on hover, this one doesn’t). -
Maintainability |
components/header-search-button.tsx:153-160
desktopButtonnow has botharia-labelandtitleplus adata-testid. In other parts of this PR, you’re standardizing on Tooltip components for visual labels rather than relying ontitle.
Keeping title here while the rest of the header uses Tooltip is inconsistent and can also lead to duplicate announcements/UX depending on the tooltip implementation and browser behavior.
- Maintainability |
components/header.tsx:97-105
The desktopUsage & Creditsbutton in the header usessr-onlytext but does not include anaria-label, whereas other icon-only buttons in this PR often include one.
Relying on nested sr-only is generally fine, but mixing approaches across the codebase makes it easier to miss accessibility requirements in future changes.
- Maintainability |
components/history.tsx:18-18
Historynow builds abuttonelement once and conditionally wraps it in aTooltipfor the header. Good refactor, but note that the tooltip content duplicates thearia-label. If copy changes later, it’s easy for tooltip text andaria-labelto diverge, which is an accessibility and UX consistency issue.
Summary of changes
What changed
- Added project learnings to
.Jules/palette.mddocumenting best practices for icon-only button UX (tooltips) and portal target stability. - Enabled tooltips app-wide by wrapping the main app content in
TooltipProviderinapp/layout.tsx. - Replaced
title-only or unlabeled icon buttons withTooltip+TooltipTrigger+TooltipContentand addedaria-label/sr-onlytext across:components/header.tsx(logo/history toggle, calendar, usage)components/history.tsx(header-only tooltip)components/chat-panel.tsx(new chat + attachment tooltip; clear attachmentaria-label)
- Fixed mobile search injection by removing the non-functional button and adding a portal container
#mobile-header-search-portalincomponents/mobile-icons-bar.tsx. - Added missing
aria-labels for multiple mobile icon buttons and added adata-testidto the desktop resolution search button.
| <TooltipProvider> | ||
| <MapContextProvider> | ||
| <MapLoadingProvider> | ||
| <Header /> | ||
| <ConditionalLottie /> | ||
| {children} | ||
| <HistorySidebar /> | ||
| <Footer /> | ||
| <Toaster /> | ||
| </MapLoadingProvider> | ||
| </MapContextProvider> | ||
| </TooltipProvider> |
There was a problem hiding this comment.
TooltipProvider is added without any configuration. Many tooltip implementations (e.g., Radix-based) default to non-zero delays and may show tooltips on touch/keyboard in ways that can feel noisy across the whole app.
Given the PR goal is UX/accessibility, it’s worth explicitly setting provider props (delay, skipDelay, disableHoverableContent) to a deliberate global behavior rather than relying on library defaults.
Suggestion
Consider configuring the provider explicitly, e.g.:
<TooltipProvider delayDuration={200} skipDelayDuration={0} disableHoverableContent>
...
</TooltipProvider>This makes the global UX predictable and avoids regressions if library defaults change. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| <a href="https://buy.stripe.com/14A3cv7K72TR3go14Nasg02" target="_blank" rel="noopener noreferrer"> | ||
| <Button variant="ghost" size="icon"> | ||
| <Button variant="ghost" size="icon" aria-label="Usage & Credits"> | ||
| <TentTree className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> | ||
| </Button> | ||
| </a> | ||
| <Button variant="ghost" size="icon" onClick={onAttachmentClick} data-testid="mobile-attachment-button"> | ||
| <Button variant="ghost" size="icon" onClick={onAttachmentClick} data-testid="mobile-attachment-button" aria-label="Attach file"> | ||
| <Paperclip className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> | ||
| </Button> |
There was a problem hiding this comment.
Mobile Usage & Credits is implemented as an <a> wrapping a <Button>. This often results in nested interactive semantics (a button inside a link), which is problematic for accessibility and can cause inconsistent click/keyboard behavior.
Since this PR is explicitly about accessibility, this should be addressed here rather than left as-is.
Suggestion
Render the button as a link (or style an anchor) instead of nesting. If your Button supports asChild:
<Button asChild variant="ghost" size="icon" aria-label="Usage & Credits">
<a href="..." target="_blank" rel="noopener noreferrer">
<TentTree ... />
</a>
</Button>Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| <div className="mobile-icons-bar-content"> | ||
| <Button variant="ghost" size="icon" onClick={handleNewChat} data-testid="mobile-new-chat-button"> | ||
| <Button variant="ghost" size="icon" onClick={handleNewChat} data-testid="mobile-new-chat-button" aria-label="New Chat"> | ||
| <Plus className="h-[1.2rem] w-[1.2rem]" /> | ||
| </Button> | ||
| <ProfileToggle /> | ||
| <MapToggle /> | ||
| <Button variant="ghost" size="icon" onClick={toggleCalendar} title="Open Calendar" data-testid="mobile-calendar-button"> | ||
| <Button variant="ghost" size="icon" onClick={toggleCalendar} aria-label="Open Calendar" data-testid="mobile-calendar-button"> | ||
| <CalendarDays className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> | ||
| </Button> | ||
| <Button variant="ghost" size="icon" data-testid="mobile-search-button"> | ||
| <Search className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> | ||
| </Button> | ||
| <div id="mobile-header-search-portal" className="contents" /> | ||
| <a href="https://buy.stripe.com/14A3cv7K72TR3go14Nasg02" target="_blank" rel="noopener noreferrer"> | ||
| <Button variant="ghost" size="icon"> | ||
| <Button variant="ghost" size="icon" aria-label="Usage & Credits"> | ||
| <TentTree className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> | ||
| </Button> | ||
| </a> | ||
| <Button variant="ghost" size="icon" onClick={onAttachmentClick} data-testid="mobile-attachment-button"> | ||
| <Button variant="ghost" size="icon" onClick={onAttachmentClick} data-testid="mobile-attachment-button" aria-label="Attach file"> | ||
| <Paperclip className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> |
There was a problem hiding this comment.
Replacing the mobile search button with a portal mount fixes the missing target issue, but it also removes the visible search affordance if the portal injection fails for any reason (timing, conditional rendering, regression elsewhere). Right now there’s no fallback UI, so mobile users could lose search entirely without a clear signal.
Suggestion
Add a lightweight fallback search button when the portal injection is absent, or at least leave a non-interactive placeholder that clearly indicates search is loading/unavailable. One pragmatic approach is to render a disabled search button until the portal content mounts (if you can detect that), or keep the old button wired to the same handler used by the portal content.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit that introduces a safe fallback while preserving the portal target.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/header-search-button.tsx (1)
150-165: 🧹 Nitpick | 🔵 TrivialInconsistent tooltip approach:
titleattribute vs. Radix<Tooltip>used elsewhere.Other header buttons (Calendar, Usage, History logo) are wrapped with
<Tooltip>/<TooltipContent>for visual tooltips, but this desktop button relies on the nativetitleattribute (line 158). This creates an inconsistent hover experience — native browser tooltips look and behave differently from the Radix tooltip popover used on sibling controls.Consider wrapping this button with
<Tooltip>to match the rest of the header, and dropping thetitleprop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header-search-button.tsx` around lines 150 - 165, The desktopButton uses a native title attribute for its hover help, causing inconsistency with sibling controls that use the Radix Tooltip; wrap the Button JSX (the desktopButton element) with the existing Tooltip components (e.g., <Tooltip> and <TooltipTrigger>/<TooltipContent> used elsewhere) and remove the title prop so the tooltip text ("Analyze current map view") is rendered via TooltipContent instead, leaving onClick={handleResolutionSearch}, disabled, data-testid and aria-label intact.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.Jules/palette.mdapp/layout.tsxcomponents/chat-panel.tsxcomponents/header-search-button.tsxcomponents/header.tsxcomponents/history.tsxcomponents/mobile-icons-bar.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
components/mobile-icons-bar.tsx (2)
components/profile-toggle.tsx (1)
ProfileToggle(9-82)components/map-toggle.tsx (1)
MapToggle(14-39)
components/header-search-button.tsx (4)
tests/map.spec.ts (1)
test(3-62)tests/responsive.spec.ts (2)
test(3-40)test(227-260)tests/header.spec.ts (2)
test(3-30)page(8-17)components/map-toggle.tsx (1)
MapToggle(14-39)
components/history.tsx (3)
components/history-toggle-context.tsx (4)
useHistoryToggle(26-30)open(17-17)isHistoryOpen(13-24)HistoryToggleContextType(5-9)components/ui/tooltip.tsx (3)
Tooltip(30-30)TooltipTrigger(30-30)TooltipContent(30-30)components/history-skelton.tsx (1)
HistorySkeleton(4-12)
components/header.tsx (1)
components/ui/tooltip.tsx (2)
Tooltip(30-30)props(17-27)
🪛 markdownlint-cli2 (0.20.0)
.Jules/palette.md
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (9)
.Jules/palette.md (1)
1-3: Verify the learning entry date.The learning is dated "2025-05-15", which is approximately 9 months before this PR was created (2026-02-17), yet this is a new file being added. This raises questions about whether:
- The date is incorrect and should be more recent
- The documentation was delayed significantly
- The entry is being backdated intentionally
Please confirm the date is accurate and matches when this learning was actually captured.
app/layout.tsx (1)
111-122: LGTM!Placing a single
TooltipProviderat the root layout level is the recommended Radix UI pattern, ensuring all nested<Tooltip>instances share consistent delay and context. Good placement insideThemeProviderso tooltips inherit theme styles.components/chat-panel.tsx (2)
177-192: LGTM!Tooltip wrapping for the New Chat button is correctly implemented with
asChildand matchingaria-label+TooltipContent.
314-314: LGTM!Good addition of
aria-labelon the clear attachment button.components/history.tsx (1)
16-42: LGTM!Clean refactor — extracting the button and conditionally wrapping with
<Tooltip>only for the header location is a good pattern that avoids unnecessary tooltip rendering in the sidebar context.components/header.tsx (2)
85-105: LGTM!Calendar and Usage tooltip wrappers follow the established pattern correctly, with matching
sr-onlytext andTooltipContent.
115-117: LGTM!Good addition of
aria-labelon the mobile usage button.components/mobile-icons-bar.tsx (2)
40-59: LGTM!Consistent
aria-labeladditions across all mobile icon buttons. The labels accurately describe each button's action.
48-48: Good fix: mobile search portal container.This
divprovides the portal target thatheader-search-button.tsxneeds to render the mobile search button viacreatePortal. This addresses the reported bug where the mobile search button was non-functional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.Jules/palette.md:
- Around line 1-7: The markdown file lacks a top-level heading and blank lines
around headings causing MD041 and MD022 failures; add a first-line H1 (e.g., a
descriptive top-level heading) so the file starts with a level-1 heading, and
ensure every existing heading (like "2025-05-15 - [Consistency in Icon-Only
Button Feedback]" and "2025-05-15 - [Portal-based Component Injection]") has a
blank line both before and after it so headings are properly surrounded by blank
lines.
In `@components/chat-panel.tsx`:
- Around line 228-246: The Paperclip icon size uses a redundant ternary
(isMobile ? 18 : 20) inside the block already guarded by !isMobile; update the
Paperclip usage in the ChatPanel render so it uses the literal 20 size (replace
Paperclip size prop with 20) to remove the dead branch; locate the JSX around
Tooltip/TooltipTrigger/Button where handleAttachmentClick, Paperclip, and
isMobile are referenced and make the size change there.
In `@components/header.tsx`:
- Around line 60-74: Replace the visually-hidden span used for the button label
with an explicit aria-label on the Button component to avoid duplicate
accessible names; specifically, on the Button (the one with variant="ghost"
size="icon" data-testid="logo-history-toggle" and onClick={toggleHistory}) add
aria-label="Toggle history" and remove the <span className="sr-only">Toggle
history</span> descendant so the Image alt="Logo" remains for the image only and
the Button has a single, clear accessible name.
---
Outside diff comments:
In `@components/header-search-button.tsx`:
- Around line 150-165: The desktopButton uses a native title attribute for its
hover help, causing inconsistency with sibling controls that use the Radix
Tooltip; wrap the Button JSX (the desktopButton element) with the existing
Tooltip components (e.g., <Tooltip> and <TooltipTrigger>/<TooltipContent> used
elsewhere) and remove the title prop so the tooltip text ("Analyze current map
view") is rendered via TooltipContent instead, leaving
onClick={handleResolutionSearch}, disabled, data-testid and aria-label intact.
| ## 2025-05-15 - [Consistency in Icon-Only Button Feedback] | ||
| **Learning:** Sighted users rely on visual tooltips (like the native `title` attribute or custom `Tooltip` components) to understand icon-only buttons. Removing these without providing a replacement degrades the UX. Accessibility improvements (`aria-label`) should complement, not replace, visual labels. | ||
| **Action:** Always ensure icon-only buttons have either a `title` attribute or are wrapped in a `Tooltip` component for visual feedback, in addition to `aria-label` for screen readers. | ||
|
|
||
| ## 2025-05-15 - [Portal-based Component Injection] | ||
| **Learning:** When using portals to inject buttons (e.g., `HeaderSearchButton`), the target container must exist in the DOM for the button to appear. If the target container is moved or replaced, the portal ID must be maintained or updated. | ||
| **Action:** When refactoring layouts that host portal targets, verify that the target IDs (like `mobile-header-search-portal`) are preserved or correctly re-implemented. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Address markdown formatting issues.
The static analysis tool flags minor markdown formatting violations:
- Headings should be surrounded by blank lines (MD022)
- First line should be a top-level heading (MD041)
📝 Proposed fix for markdown formatting
+# Palette Learnings
+
## 2025-05-15 - [Consistency in Icon-Only Button Feedback]
+
**Learning:** Sighted users rely on visual tooltips (like the native `title` attribute or custom `Tooltip` components) to understand icon-only buttons. Removing these without providing a replacement degrades the UX. Accessibility improvements (`aria-label`) should complement, not replace, visual labels.
+
**Action:** Always ensure icon-only buttons have either a `title` attribute or are wrapped in a `Tooltip` component for visual feedback, in addition to `aria-label` for screen readers.
## 2025-05-15 - [Portal-based Component Injection]
+
**Learning:** When using portals to inject buttons (e.g., `HeaderSearchButton`), the target container must exist in the DOM for the button to appear. If the target container is moved or replaced, the portal ID must be maintained or updated.
+
**Action:** When refactoring layouts that host portal targets, verify that the target IDs (like `mobile-header-search-portal`) are preserved or correctly re-implemented.📝 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.
| ## 2025-05-15 - [Consistency in Icon-Only Button Feedback] | |
| **Learning:** Sighted users rely on visual tooltips (like the native `title` attribute or custom `Tooltip` components) to understand icon-only buttons. Removing these without providing a replacement degrades the UX. Accessibility improvements (`aria-label`) should complement, not replace, visual labels. | |
| **Action:** Always ensure icon-only buttons have either a `title` attribute or are wrapped in a `Tooltip` component for visual feedback, in addition to `aria-label` for screen readers. | |
| ## 2025-05-15 - [Portal-based Component Injection] | |
| **Learning:** When using portals to inject buttons (e.g., `HeaderSearchButton`), the target container must exist in the DOM for the button to appear. If the target container is moved or replaced, the portal ID must be maintained or updated. | |
| **Action:** When refactoring layouts that host portal targets, verify that the target IDs (like `mobile-header-search-portal`) are preserved or correctly re-implemented. | |
| # Palette Learnings | |
| ## 2025-05-15 - [Consistency in Icon-Only Button Feedback] | |
| **Learning:** Sighted users rely on visual tooltips (like the native `title` attribute or custom `Tooltip` components) to understand icon-only buttons. Removing these without providing a replacement degrades the UX. Accessibility improvements (`aria-label`) should complement, not replace, visual labels. | |
| **Action:** Always ensure icon-only buttons have either a `title` attribute or are wrapped in a `Tooltip` component for visual feedback, in addition to `aria-label` for screen readers. | |
| ## 2025-05-15 - [Portal-based Component Injection] | |
| **Learning:** When using portals to inject buttons (e.g., `HeaderSearchButton`), the target container must exist in the DOM for the button to appear. If the target container is moved or replaced, the portal ID must be maintained or updated. | |
| **Action:** When refactoring layouts that host portal targets, verify that the target IDs (like `mobile-header-search-portal`) are preserved or correctly re-implemented. |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.Jules/palette.md around lines 1 - 7, The markdown file lacks a top-level
heading and blank lines around headings causing MD041 and MD022 failures; add a
first-line H1 (e.g., a descriptive top-level heading) so the file starts with a
level-1 heading, and ensure every existing heading (like "2025-05-15 -
[Consistency in Icon-Only Button Feedback]" and "2025-05-15 - [Portal-based
Component Injection]") has a blank line both before and after it so headings are
properly surrounded by blank lines.
| {!isMobile && ( | ||
| <Button | ||
| type="button" | ||
| variant={'ghost'} | ||
| size={'icon'} | ||
| className={cn( | ||
| 'absolute top-1/2 transform -translate-y-1/2 left-3' | ||
| )} | ||
| onClick={handleAttachmentClick} | ||
| data-testid="desktop-attachment-button" | ||
| > | ||
| <Paperclip size={isMobile ? 18 : 20} /> | ||
| </Button> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| type="button" | ||
| variant={'ghost'} | ||
| size={'icon'} | ||
| className={cn( | ||
| 'absolute top-1/2 transform -translate-y-1/2 left-3' | ||
| )} | ||
| onClick={handleAttachmentClick} | ||
| data-testid="desktop-attachment-button" | ||
| aria-label="Attach file" | ||
| > | ||
| <Paperclip size={isMobile ? 18 : 20} /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent>Attach file</TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
Dead branch: isMobile ternary always evaluates to false here.
Line 228 guards this block with !isMobile, so isMobile ? 18 : 20 on line 242 will always resolve to 20. Simplify to a literal.
Proposed fix
- <Paperclip size={isMobile ? 18 : 20} />
+ <Paperclip size={20} />📝 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.
| {!isMobile && ( | |
| <Button | |
| type="button" | |
| variant={'ghost'} | |
| size={'icon'} | |
| className={cn( | |
| 'absolute top-1/2 transform -translate-y-1/2 left-3' | |
| )} | |
| onClick={handleAttachmentClick} | |
| data-testid="desktop-attachment-button" | |
| > | |
| <Paperclip size={isMobile ? 18 : 20} /> | |
| </Button> | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <Button | |
| type="button" | |
| variant={'ghost'} | |
| size={'icon'} | |
| className={cn( | |
| 'absolute top-1/2 transform -translate-y-1/2 left-3' | |
| )} | |
| onClick={handleAttachmentClick} | |
| data-testid="desktop-attachment-button" | |
| aria-label="Attach file" | |
| > | |
| <Paperclip size={isMobile ? 18 : 20} /> | |
| </Button> | |
| </TooltipTrigger> | |
| <TooltipContent>Attach file</TooltipContent> | |
| </Tooltip> | |
| {!isMobile && ( | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <Button | |
| type="button" | |
| variant={'ghost'} | |
| size={'icon'} | |
| className={cn( | |
| 'absolute top-1/2 transform -translate-y-1/2 left-3' | |
| )} | |
| onClick={handleAttachmentClick} | |
| data-testid="desktop-attachment-button" | |
| aria-label="Attach file" | |
| > | |
| <Paperclip size={20} /> | |
| </Button> | |
| </TooltipTrigger> | |
| <TooltipContent>Attach file</TooltipContent> | |
| </Tooltip> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/chat-panel.tsx` around lines 228 - 246, The Paperclip icon size
uses a redundant ternary (isMobile ? 18 : 20) inside the block already guarded
by !isMobile; update the Paperclip usage in the ChatPanel render so it uses the
literal 20 size (replace Paperclip size prop with 20) to remove the dead branch;
locate the JSX around Tooltip/TooltipTrigger/Button where handleAttachmentClick,
Paperclip, and isMobile are referenced and make the size change there.
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button variant="ghost" size="icon" onClick={toggleHistory} data-testid="logo-history-toggle"> | ||
| <Image | ||
| src="/images/logo.svg" | ||
| alt="Logo" | ||
| width={20} | ||
| height={20} | ||
| className="h-5 w-auto" | ||
| /> | ||
| <span className="sr-only">Toggle history</span> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent>Toggle history</TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using aria-label instead of sr-only span on the logo button.
The button contains an <Image alt="Logo"> and a <span className="sr-only">Toggle history</span>. Screen readers may announce both — "Logo Toggle history" — because the accessible name is computed from all descendant text content. An explicit aria-label="Toggle history" on the <Button> would give a single, clean accessible name, consistent with how other buttons in this PR use aria-label.
Proposed fix
- <Button variant="ghost" size="icon" onClick={toggleHistory} data-testid="logo-history-toggle">
+ <Button variant="ghost" size="icon" onClick={toggleHistory} data-testid="logo-history-toggle" aria-label="Toggle history">
<Image
src="/images/logo.svg"
- alt="Logo"
+ alt=""
width={20}
height={20}
className="h-5 w-auto"
/>
- <span className="sr-only">Toggle history</span>
</Button>📝 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 variant="ghost" size="icon" onClick={toggleHistory} data-testid="logo-history-toggle"> | |
| <Image | |
| src="/images/logo.svg" | |
| alt="Logo" | |
| width={20} | |
| height={20} | |
| className="h-5 w-auto" | |
| /> | |
| <span className="sr-only">Toggle history</span> | |
| </Button> | |
| </TooltipTrigger> | |
| <TooltipContent>Toggle history</TooltipContent> | |
| </Tooltip> | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <Button variant="ghost" size="icon" onClick={toggleHistory} data-testid="logo-history-toggle" aria-label="Toggle history"> | |
| <Image | |
| src="/images/logo.svg" | |
| alt="" | |
| width={20} | |
| height={20} | |
| className="h-5 w-auto" | |
| /> | |
| </Button> | |
| </TooltipTrigger> | |
| <TooltipContent>Toggle history</TooltipContent> | |
| </Tooltip> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/header.tsx` around lines 60 - 74, Replace the visually-hidden span
used for the button label with an explicit aria-label on the Button component to
avoid duplicate accessible names; specifically, on the Button (the one with
variant="ghost" size="icon" data-testid="logo-history-toggle" and
onClick={toggleHistory}) add aria-label="Toggle history" and remove the <span
className="sr-only">Toggle history</span> descendant so the Image alt="Logo"
remains for the image only and the Button has a single, clear accessible name.
2 participants
This PR improves the overall accessibility and user experience of the application by ensuring all icon-only buttons are properly labeled for both sighted and screen-reader users. Key changes include:
TooltipProvider.aria-labelandsr-onlytext to buttons for screen reader support.PR created automatically by Jules for task 2808992707312428823 started by @ngoiyaeric
Summary by CodeRabbit
New Features
Documentation