fix: improve SVG icon accessibility#445
Conversation
|
@SiyaSatija is attempting to deploy a commit to the magic-peach1's projects Team on Vercel. A member of the Team first needs to authorize it. |
👋 Thanks for your PR, @SiyaSatija!Welcome to Reframe — a browser-based video editor built for everyone 🎬 What happens next
Quick checklist
Useful links
Happy coding! 🎉 |
|
There was a problem hiding this comment.
Pull request overview
Adds aria-hidden="true" to decorative Lucide icons across the video-editor UI so screen readers skip them, eliminating redundant announcements where adjacent visible text or sr-only labels already convey meaning (issue #171).
Changes:
- Mark decorative Lucide icons (
Scissors,RotateCw,Volume2,Layers,Crop,Github,Download, etc.) asaria-hidden="true". - Covers icons accompanied by visible labels (Section titles, buttons like EXPORT/Download/New) and by
sr-onlytext (rotate-degree buttons, framing buttons, audio toggle). - No behavioral/logic changes; purely accessibility-attribute additions.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/VideoEditor.tsx | aria-hidden added to Section icons, AlertTriangle, Zap (EXPORT button), and Github link icon. |
| src/components/RotateControl.tsx | aria-hidden added to rotating RotateCw icon (sr-only label already present). |
| src/components/PresetSelector.tsx | aria-hidden added to Settings2 icon. |
| src/components/FramingControl.tsx | aria-hidden added to per-mode Icon (sr-only describes mode). |
| src/components/FileUpload.tsx | aria-hidden added to Film and FolderOpen icons. |
| src/components/ExportSettings.tsx | aria-hidden added to SlidersHorizontal next to “Quality” label. |
| src/components/DownloadResult.tsx | aria-hidden added to Download and RotateCcw button icons. |
| src/components/AudioSpeedControl.tsx | aria-hidden added to Volume2/VolumeX toggle icons and Gauge label icon. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| <Section icon={<Crop size={12} />} title="Framing" delay={100}> | ||
|
|
||
| <Section icon={<Crop size={12} aria-hidden="true" />} title="Framing" delay={100}> |
| className="flex items-center gap-1.5 text-[11px] font-heading font-medium text-[var(--muted)] hover:text-film-600 transition-colors" | ||
| > | ||
| <Github size={13} /> | ||
| <Github size={13} aria-hidden="true"/> |
| <Download size={15} aria-hidden="true" /> | ||
| Download {result.format.toUpperCase()} | ||
| </a> | ||
| <button | ||
| type="button" | ||
| onClick={onReset} | ||
| className="flex items-center gap-2 px-4 py-3 border border-[var(--border)] text-[var(--muted)] text-sm rounded-lg hover:bg-[var(--bg)] transition-colors" | ||
| > | ||
| <RotateCcw size={14} /> | ||
| <RotateCcw size={14} aria-hidden="true" /> |
| > | ||
| <Settings2 | ||
| size={20} | ||
| size={20} aria-hidden="true" |
|
Hey @SiyaSatija! The build is failing with a JSX syntax error in Error — line 101: This is a mismatched or prematurely closed JSX tag around line 100-101 in the component's render output. A Fix: Check the JSX nesting in the speed description section (around lines 95–110) of |
|
Hi @SiyaSatija 👋 This PR has a merge conflict with git fetch upstream && git merge upstream/main
# resolve any conflicts, then:
git pushOnce conflicts are resolved and CI passes, this will be ready to merge. 🙂 |
|
Hey @SiyaSatija, this PR has merge conflicts with main. Please merge/rebase main into your branch and resolve the conflicts before we can review it. Run: |
|
Hey @SiyaSatija! The SVG accessibility improvements look good, but this PR now has conflicts with git fetch origin && git rebase origin/mainResolve any conflicts in those files (keep the new changes from main AND your accessibility improvements), then push. Once CI passes, it'll be ready to merge! |
|
Hey @SiyaSatija! The build check is failing for this PR. Please review the build logs, fix the error, and push an update. Once the build passes and the code looks clean, this will be ready to merge. Let me know if you need help identifying the issue! |
|
Hey @SiyaSatija! Improving SVG icon accessibility is a great contribution. This PR has several issues:
Please rebase and fix both build and lint errors before this can be reviewed. |
|
Hey @SiyaSatija! This PR has CI check failures (build/lint/typecheck) and merge conflicts with
git fetch origin
git rebase origin/main
# fix any errors, then:
git push --force-with-leaseOnce all checks pass, we'll review and merge! |
|
Hey @SiyaSatija! This PR had merge conflicts with |
|
Hey @SiyaSatija! I've resolved the merge conflicts and pushed, but the build/lint/typecheck checks are still failing. Please check the build logs at the CI check links above and fix the errors. Common issues to look for: TypeScript errors, unused imports, or lint violations. Once the build passes, we'll review and merge! |
cc21ee2 to
88a517a
Compare
|
Hey @SiyaSatija! The build was failing due to unresolved conflict markers in |
|
Merged! ✅ All checks pass. Thanks @SiyaSatija — adding |
…gic-peach#445) Squash merged. Build/lint/typecheck pass. Adds aria-hidden='true' to decorative SVG icons in DownloadResult and FramingControl - good accessibility improvement.
#171
Adds accessibility improvements for decorative SVG icons by applying
aria-hidden="true"to non-informative Lucide icons that already have accompanying visible or screen-reader text. This prevents redundant announcements from screen readers and improves overall accessibility.