feat: [UI] Update Timeline icon for "Auto Snapping"#420
Conversation
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
|
@chrissshan is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe snapping toggle button in the timeline toolbar now uses a "Magnet" icon for both enabled and disabled states, with color indicating status. Previous "Lock" and "LockOpen" icons were removed. Minor formatting adjustments were made in marker rendering and click-to-seek calculations without altering any core logic or behavior. Changes
Estimated code review effort1 (~2 minutes) Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/components/editor/timeline.tsx(4 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
apps/web/src/components/editor/timeline.tsx (1)
Learnt from: simonorzel26
PR: #324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded ml-48 class in addition to the calculated leftPosition for proper alignment. This is intentional and needed for the new timeline layout.
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
apps/web/src/components/editor/timeline.tsx (1)
Learnt from: simonorzel26
PR: #324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded ml-48 class in addition to the calculated leftPosition for proper alignment. This is intentional and needed for the new timeline layout.
🔇 Additional comments (1)
apps/web/src/components/editor/timeline.tsx (1)
922-935: LGTM – marker style refactor is functionally equivalentThe refactor to a single-line template literal retains the same conditional logic and should not affect rendering.
| <Button variant="text" size="icon" onClick={toggleSnapping}> | ||
| {snappingEnabled ? ( | ||
| <LockOpen className="h-4 w-4 text-primary" /> | ||
| <Magnet className="h-4 w-4 text-primary" /> | ||
| ) : ( | ||
| <Lock className="h-4 w-4" /> | ||
| <Magnet className="h-4 w-4" /> | ||
| )} | ||
| </Button> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding a non-color cue for the snapping toggle’s state
Using the exact same Magnet glyph for both enabled and disabled states and relying solely on the primary-color tint to convey “on” violates WCAG 1.4.1 (“Use of Color”).
A simple alternative is to convey state via the tooltip label or by swapping to the Magnet+Slash icon (exists in lucide-react as MagnetOff) when the feature is disabled.
- {snappingEnabled ? (
- <Magnet className="h-4 w-4 text-primary" />
- ) : (
- <Magnet className="h-4 w-4" />
- )}
+ {snappingEnabled ? (
+ <Magnet className="h-4 w-4 text-primary" />
+ ) : (
+ <MagnetOff className="h-4 w-4 text-muted-foreground" />
+ )}If you prefer to keep a single icon, at minimum change the tooltip to “Enable Auto Snapping” / “Disable Auto Snapping” so screen-reader and color-blind users can distinguish the state.
#!/bin/bash
Search for all Magnet icon usage in the timeline component
rg "Magnet" -n apps/web/src/components/editor/timeline.tsx
Locate the toggleSnapping handler to see surrounding JSX (including any Tooltip)
rg "toggleSnapping" -n apps/web/src/components/editor/timeline.tsx
Dump the region around the snapping toggle button to inspect labels/tooltips
sed -n '750,860p' apps/web/src/components/editor/timeline.tsx
🏁 Script executed:
#!/bin/bash
set -e
# Search for Tooltip wrapper around the snapping toggle
rg "<Tooltip" -n apps/web/src/components/editor/timeline.tsx
# Search for any aria-label or title on the snapping toggle button
rg "aria-label" -n apps/web/src/components/editor/timeline.tsx
rg "title=" -n apps/web/src/components/editor/timeline.tsx
# Show context around the toggleSnapping handler to inspect labels and icons
rg "onClick={toggleSnapping}" -n -C 5 apps/web/src/components/editor/timeline.tsxLength of output: 1710
🏁 Script executed:
#!/bin/bash
# Dump context around the snapping toggle to inspect icon, tooltip, and labels
sed -n '780,840p' apps/web/src/components/editor/timeline.tsxLength of output: 2179
Add a non-color cue to the Auto Snapping toggle
Relying solely on the text-primary tint to indicate “on” can be inaccessible for color-blind users and violates WCAG 1.4.1. We should either swap to a “disabled” icon when snapping is off or at minimum provide dynamic labels (tooltip/aria-label) that make the state clear without color.
• File: apps/web/src/components/editor/timeline.tsx
• Location: within the <Button onClick={toggleSnapping}>…</Button> block (~lines 809–812)
Suggested diff:
- <Button variant="text" size="icon" onClick={toggleSnapping}>
- {snappingEnabled ? (
- <Magnet className="h-4 w-4 text-primary" />
- ) : (
- <Magnet className="h-4 w-4" />
- )}
- </Button>
- <TooltipContent>Auto snapping</TooltipContent>
+ <Button
+ variant="text"
+ size="icon"
+ onClick={toggleSnapping}
+ aria-label={snappingEnabled ? "Disable Auto Snapping" : "Enable Auto Snapping"}
+ >
+ {snappingEnabled ? (
+ <Magnet className="h-4 w-4 text-primary" />
+ ) : (
+ <MagnetOff className="h-4 w-4 text-muted-foreground" />
+ )}
+ </Button>
+ <TooltipContent>
+ {snappingEnabled ? "Disable Auto Snapping" : "Enable Auto Snapping"}
+ </TooltipContent>This ensures both a distinct icon and clear labels for screen-reader and color-blind users.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/src/components/editor/timeline.tsx around lines 806 to 812, the Auto
Snapping toggle button relies only on color to indicate its state, which is
inaccessible for color-blind users. To fix this, add a non-color visual cue by
swapping the icon to a "disabled" or different style when snapping is off, and
include dynamic aria-labels or tooltips that clearly describe the toggle state
for screen readers and all users.
chrissshan
changed the title
…eline-snapping-icon feat: [UI] Update Timeline icon for "Auto Snapping"
2 participants
Description
Swap the Timeline icon for "Auto Snapping" from
LocktoMagnet.Lock gives the impression that the timeline will be locked from changes. Magnet is a clearer intention of what expected behavior would be.
Type of change
New feature (non-breaking change which adds functionality)
How Has This Been Tested?
Screenshots (if applicable)
Before change:
After change:
Checklist:
Additional context
none
Summary by CodeRabbit