fix(design): give the icon sprite a viewBox so the icons are not cropped - #4
Merged
Conversation
The sprite drew each icon inside <g id="i-book">. A <g> groups; it does not establish a viewport and carries no viewBox, and none of the 81 consuming <svg class="icon"> elements had one either. With no viewport an SVG maps one user unit to one pixel, so 24 units of artwork inside a 13px box rendered as the top-left corner of the drawing — every icon clipped, and each one offset differently depending on where its art sits in the 24x24 grid. <g> becomes <symbol viewBox="0 0 24 24">. With <use>, the symbol's viewBox establishes the viewport and scales the artwork into whatever box the consumer sizes, so one change to the sprite fixes all 81 usages and no individual <svg> needed touching. Second defect the first one was hiding: only .icon carried flex: 0 0 auto, while .icon-sm and .icon-lg are used standalone everywhere. An icon in a tight flex row is a flex item, so it would have compressed horizontally into an ellipse in buttons with long labels and in the sources table. All three now carry it, plus display: block to drop the baseline gap outside flex contexts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GRMARM7Kd1HoRnGLVqSkAe
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe draft design prevents icon variants from shrinking in flex layouts. It also converts inline SVG sprite groups to 24×24 symbols while preserving existing artwork and identifiers. ChangesIcon sprite update
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One commit. Follows #3, which merged before this fix was pushed, so
maincurrently carries the draft with every icon clipped.The defect
The sprite drew each icon inside
<g id="i-book">. A<g>groups elements; it establishes no viewport and carries noviewBox. None of the 81 consuming<svg class="icon">elements had one either.With no viewport an SVG maps one user unit to one pixel, so 24 units of Lucide artwork inside a 13px box rendered as the top-left corner of the drawing. Every icon was clipped, and each one appeared offset differently depending on where its art happens to sit in the 24×24 grid — which is what reads, from outside, as "the icons are not aligned".
The fix
<g id="i-x">becomes<symbol id="i-x" viewBox="0 0 24 24">. With<use>, the symbol'sviewBoxestablishes the viewport and scales the artwork into whatever box the consumer sizes, so one change in the sprite fixes all 81 usages and no individual<svg>needed touching.A second defect the first one was hiding
Only
.iconcarriedflex: 0 0 auto, and.icon-sm/.icon-lgare used standalone throughout. An icon in a flex row is a flex item, so without it the glyph compresses horizontally into an ellipse whenever the row is tight — buttons with long labels, the sources table, the dialog headings. All three classes now carry it, plusdisplay: blockto drop the baseline gap in any non-flex context.Verified
22 symbols defined, 22 referenced, none orphaned in either direction, every one carrying a
viewBox, zero<g>remaining, HTML balanced.That is a structural check and it is not what found the bug. Nothing in this repository renders the page, so the missing
viewBoxpassed every automated check I have and was caught by a person looking at the file. Open it rather than trusting the list.Stroke width is left at 1.75, which is about 0.95px effective at 13px — the thin stroke that matches the density task 8.1 asks for. Now that the icons actually draw, that is a number worth judging on a real screen.
🤖 Generated with Claude Code
https://claude.ai/code/session_01GRMARM7Kd1HoRnGLVqSkAe
Summary by CodeRabbit