Skip to content

refactor: update media upload button and tooltip in media panel, so i… #557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: update media upload button and tooltip in media panel, so i… #557

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 20 additions & 15 deletions apps/web/src/components/editor/media-panel/views/media.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { processMediaFiles } from "@/lib/media-processing";
import { useMediaStore, type MediaItem } from "@/stores/media-store";
import {
ArrowDown01,
CloudUpload,
Upload,
Grid2X2,
Image,
List,
Expand Down Expand Up @@ -292,22 +292,27 @@ export function MediaView() {
<div className="p-3 pb-2 bg-panel">
{/* Search and filter controls */}
<div className="flex items-center gap-2">
<Button
variant="outline"
size="lg"
onClick={handleFileSelect}
disabled={isProcessing}
className="!bg-background px-4 flex-1 justify-center items-center h-9 opacity-100 hover:opacity-75 transition-opacity"
>
{isProcessing ? (
<Loader2 className="h-4 w-4 animate-spin" />
) : (
<CloudUpload className="h-4 w-4" />
)}
<span>Upload</span>
</Button>
<div className="flex items-center gap-0">
<TooltipProvider>
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="text"
size="icon"
onClick={handleFileSelect}
disabled={isProcessing}
>
{isProcessing ? (
<Loader2 className="!size-4 animate-spin" />
) : (
<Upload className="!size-4" />
)}
</Button>
</TooltipTrigger>
Comment on lines +299 to +311
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add button type and accessible name; include titles on icons

  • Buttons must declare type to avoid unintended form submissions.
  • Icon-only buttons need an accessible name; tooltips do not provide an accessible name for assistive tech.
  • Our guidelines require a title element for SVG icons in TSX/JSX.

Apply the following changes:

  • Add type="button" and a descriptive aria-label to the Button.
  • Add title to both icons (and optionally aria-hidden to avoid double announcements).
  • Optionally normalize icon stroke width to match adjacent controls and reduce visual weight.
-                    <Button
-                      variant="text"
-                      size="icon"
-                      onClick={handleFileSelect}
-                      disabled={isProcessing}
-                    >
-                      {isProcessing ? (
-                        <Loader2 className="!size-4 animate-spin" />
-                      ) : (
-                        <Upload className="!size-4" />
-                      )}
+                    <Button
+                      variant="text"
+                      size="icon"
+                      type="button"
+                      aria-label={isProcessing ? "Uploading…" : "Upload media"}
+                      onClick={handleFileSelect}
+                      disabled={isProcessing}
+                    >
+                      {isProcessing ? (
+                        <Loader2
+                          className="!size-4 animate-spin"
+                          strokeWidth={1.5}
+                          aria-hidden="true"
+                          title="Uploading…"
+                        />
+                      ) : (
+                        <Upload
+                          className="!size-4"
+                          strokeWidth={1.5}
+                          aria-hidden="true"
+                          title="Upload media"
+                        />
+                      )}
                     </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.

Suggested change
<Button
variant="text"
size="icon"
onClick={handleFileSelect}
disabled={isProcessing}
>
{isProcessing ? (
<Loader2 className="!size-4 animate-spin" />
) : (
<Upload className="!size-4" />
)}
</Button>
</TooltipTrigger>
<Button
variant="text"
size="icon"
type="button"
aria-label={isProcessing ? "Uploading…" : "Upload media"}
onClick={handleFileSelect}
disabled={isProcessing}
>
{isProcessing ? (
<Loader2
className="!size-4 animate-spin"
strokeWidth={1.5}
aria-hidden="true"
title="Uploading…"
/>
) : (
<Upload
className="!size-4"
strokeWidth={1.5}
aria-hidden="true"
title="Upload media"
/>
)}
</Button>
🤖 Prompt for AI Agents 
In apps/web/src/components/editor/media-panel/views/media.tsx around lines 299
to 311, the icon-only Button is missing a button type and accessible name and
the SVG icons lack title attributes; update the Button to include type="button"
and a descriptive aria-label (e.g., aria-label="Upload media" or similar), add a
<title> to each SVG icon (and set aria-hidden="true" on the SVGs if you want to
avoid duplicate announcements), and optionally normalize the icons' strokeWidth
to match adjacent controls for consistent visual weight.

<TooltipContent>
<p>Upload media</p>
</TooltipContent>
</Tooltip>
Copy link

Choose a reason for hiding this comment

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

Missing closing </Tooltip> tag after the view mode button's TooltipContent, causing invalid JSX structure when the next <Tooltip> opens.

View Details
📝 Patch Details 
diff --git a/apps/web/src/components/editor/media-panel/views/media.tsx b/apps/web/src/components/editor/media-panel/views/media.tsx
index ec85507..abca8b1 100644
--- a/apps/web/src/components/editor/media-panel/views/media.tsx
+++ b/apps/web/src/components/editor/media-panel/views/media.tsx
@@ -343,7 +343,8 @@ export function MediaView() {
                         : "Switch to grid view"}
                     </p>
                   </TooltipContent>
-                  <Tooltip>
+                </Tooltip>
+                <Tooltip>
                     <DropdownMenu>
                       <TooltipTrigger asChild>
                         <DropdownMenuTrigger asChild>

Analysis

The tooltip structure for the view mode button (starting at line 316) is not properly closed before a new tooltip opens at line 346. The current structure has:

  1. <Tooltip> opens at line 316 (view mode button)
  2. </TooltipContent> closes at line 345
  3. <Tooltip> opens again at line 346 (dropdown button)

However, there's no </Tooltip> tag to close the view mode button's tooltip before the dropdown tooltip starts. This creates invalid JSX structure where tooltips are improperly nested, which will cause React rendering errors or compilation failures.

The structure should be:

</TooltipContent>
</Tooltip>  // ← This is missing
<Tooltip>

Recommendation

Add the missing </Tooltip> closing tag after line 345 (</TooltipContent>) and before line 346 (<Tooltip>). The corrected structure should be:

</TooltipContent>
</Tooltip>
<Tooltip>

<Tooltip>
<TooltipTrigger asChild>
<Button
Expand Down