Skip to content

🎨 Palette: Improve accessibility and tooltips for icon-only buttons - #2

Open
Marcantoinetroubat wants to merge 1 commit into
mainfrom
jules/ux-accessibility-tooltips-2928835206961848150
Open

Marcantoinetroubat wants to merge 1 commit into
mainfrom
jules/ux-accessibility-tooltips-2928835206961848150

Conversation

@Marcantoinetroubat

Copy link
Copy Markdown
Owner

💡 What: Added missing title and aria-label attributes to several icon-only buttons in the chat message context menus (Fork.tsx, MinimalHoverButtons.tsx) and the MCP server status component (MCPServerStatusIcon.tsx).
🎯 Why: To ensure that mouse users get visual tooltip feedback on hover, and screen reader users get proper context for what the buttons do, maintaining consistency with existing components like HoverButtons.tsx.
♿ Accessibility: Improved screen reader support for the copy button and added hover tooltips to the fork, connect, and configure buttons.


PR created automatically by Jules for task 2928835206961848150 started by @Marcantoinetroubat

Co-authored-by: Marcantoinetroubat <171517515+Marcantoinetroubat@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves accessibility and UI tooltips by adding missing title or aria-label attributes to buttons in Fork.tsx, MinimalHoverButtons.tsx, and MCPServerStatusIcon.tsx. Feedback is provided regarding a React anti-pattern in MCPServerStatusIcon.tsx, where a module-scoped variable is used to share the localize hook function across components. It is recommended to call useLocalize() directly inside each functional component to prevent potential runtime crashes and stale closures.

onClick={onConfigClick}
className="flex size-6 items-center justify-center rounded p-1 hover:bg-surface-secondary"
aria-label={localize('com_nav_mcp_connect_server', { 0: serverName })}
title={localize('com_nav_mcp_connect_server', { 0: serverName })}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

⚠️ React Anti-Pattern: Module-Scoped Hook Reference\n\nUsing a module-scoped variable (let localize) to store the return value of a hook (useLocalize) and sharing it across multiple components is a major React anti-pattern.\n\n#### Why this is problematic:\n1. Runtime Crashes / Test Failures: If ConnectButton, ConfigureButton, or LoadingStatusIcon are rendered or tested in isolation (without rendering MCPServerStatusIcon first), localize will be undefined, leading to a runtime crash (TypeError: localize is not a function).\n2. Stale Closures & Race Conditions: In React's concurrent rendering or when the language/locale changes dynamically, sharing a single mutable module-level variable across all instances of these components can lead to stale translations or race conditions.\n\n#### Recommended Solution:\nSince ConnectButton, ConfigureButton, and other sub-components are standard functional components, they should call useLocalize() directly inside their own bodies. This is idiomatic, safe, and eliminates the need for the module-scoped let localize variable entirely.\n\nHere is how you can refactor the file:\n\ntypescript\n// 1. Remove the module-level declaration at the top of the file:\n// let localize: ReturnType<typeof useLocalize>; // <-- Delete this\n\n// 2. Call the hook directly inside each functional component:\nfunction ConnectButton({ serverName, onConfigClick }: StatusIconProps) {\n const localize = useLocalize();\n return (\n <button\n type="button"\n onClick={onConfigClick}\n className="flex size-6 items-center justify-center rounded p-1 hover:bg-surface-secondary"\n aria-label={localize('com_nav_mcp_connect_server', { 0: serverName })}\n title={localize('com_nav_mcp_connect_server', { 0: serverName })}\n >\n <PlugZap className="size-4 text-text-secondary" aria-hidden="true" />\n </button>\n );\n}\n\nfunction ConfigureButton({ serverName, onConfigClick }: StatusIconProps) {\n const localize = useLocalize();\n return (\n <button\n type="button"\n onClick={onConfigClick}\n className="flex size-6 items-center justify-center rounded p-1 hover:bg-surface-secondary"\n aria-label={localize('com_nav_mcp_configure_server', { 0: serverName })}\n title={localize('com_nav_mcp_configure_server', { 0: serverName })}\n >\n <SlidersHorizontal className="size-4 text-text-secondary" aria-hidden="true" />\n </button>\n );\n}\n

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant