Shared SVG icon mechanism + embed the provided icon set across Watch/Star/Fork/Clone/bell/theme controls #465
Labels
No labels
actions
bug
cli
duplicate
enhancement
fork
forum
git storage
help wanted
insights
invalid
issues
moderation
oidc
ownership transfer
packages
pr/merge protection rules
projects
pull requests
question
releases
sponsorships
tags
webhooks
wiki
wontfix
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
crueber/walhub#465
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What's requested
Add a shared, embedded SVG icon mechanism for the repo header controls and the site chrome, and wire the 10 provided icons (delivered as files, embedded verbatim — see "Icon assets") into the six controls: Watch toggle, Star toggle, Fork pill, Clone trigger, notification bell, and theme toggle. Every control gets a real icon with an on/off state mapping where one exists; all emoji/unicode glyphs go away.
Icon files are provided at
/tmp/svg-icons/*.svg(all sized1em×1em, all paint viafill="currentColor"/stroke="currentColor"— they inherit text color and scale with font size, which is what makes one shared mechanism possible):watch-on.svg/watch-off.svg(eye / closed eye, viewBox 0 0 16 16)star-on.svg/star-off.svg(filled / outline star, viewBox 0 0 24 24)fork.svg(single icon, viewBox 0 0 1200 1200)clone.svg(cloud-download, viewBox 0 0 1024 1024)notify-on.svg/notify-off.svg(bell, viewBox 0 0 1024 1024)light-mode.svg/dark-mode.svg(sun / moon, viewBox 0 0 24 24)Evidence (current tree, SHA
06c9743)Six controls, five of them text/emoji glyphs today:
web/src/pages/Repo.jsx:369★), no on/off distinctionweb/src/pages/Repo.jsx:214web/src/pages/Repo.jsx:636{n} Fork— no icon at allweb/src/pages/Repo.jsx:99Clone— label-only<summary>web/src/components/NotificationTray.jsx:88<span></span>— emoji; unread badge is a separate absolute-positioned circleweb/src/App.jsx:113-121☀/☾) via<Show>The header action strip already speaks one idiom (issue #447:
btn px-2 py-1 text-sm, count LEFT of label). The icons must compose into that idiom, not redefine it.Architecture notes
Shared icon mechanism (do this once, consume everywhere). Create
web/src/lib/icons.jsxexporting one component, e.g.:Record<string, JSX>map of raw svg bodies or a vite raw-import?raw+innerHTML); keep each file'sviewBoxas-is so mixed viewport sizes (16/24/512/1024/1200 units) all scale through the existingwidth="1em" height="1em"attributes.fill/stroke="currentColor"— no color literals in the icon layer; state coloring comes from the control's own classes (primaryon toggles is the existing on-state signal).align-baseline,margin-right, etc.) belongs to the consumer's classes or a shared.btn-icon-style rule inweb/css/wal.css, not per-icon CSS. Recommend one utility (e.g.iconclass:display:inline-block; vertical-align:-0.125em;plus a flex gap on the buttons) so glyphs and labels align identically across all six controls.Placement + state mapping per control:
WatchToggle)watch-on/watch-offwatch-on; not watching →watch-off. KeepclassList={{ primary: watching }}+aria-pressedas the other half of the state signal.StarToggle)star-on/star-offstar-on; not →star-off. Same pairing withprimary/aria-pressed.forkCloneMenu<summary>)clone<summary>.NotificationTraytrigger)notify-on/notify-offnotify-on; 0 →notify-off. The unread badge circle stays as-is, positioned over the icon (no idiom change — #446's badge direction was already settled as absorbed).App.jsx)light-mode/dark-mode<Show>flips the glyph:theme() === "dark"→ showdark-mode(moon, action = go light is the current semantics — preserve the existing semantics exactly; planner's call if a swap reads better, note it). Replace☀/☾spans; keeparia-hiddenon the icon.Non-BMP note for the implementer: the Watch eye and bell emoji are 4-byte characters — removing them also removes a latent Forgejo-MySQL charset footgun from any future server-side echo of these strings.
Acceptance criteria
web/src/lib/icons.jsx(or equivalent) exists; all six controls import the one shared mechanism — no inline<svg>duplicated per page, no emoji/unicode glyph left in any of the six controls (grep for the five old glyphs inweb/srccomes back clean)primaryclass) so the on/off state is not color-onlybtn px-2 py-1 text-smpills, counts still LEFT of labels, one consistent gap mechanism (see #463 — coordinate, don't re-style)aria-liveandaria-labelbehavior unchangedaria-hidden="true"on decorative icons;aria-pressed/aria-labels on toggles unchanged in text (they are the accessible state signal)text-sm(header pills) and default (chrome buttons) sizes; no vertical misalignment between1emicons of differing viewBoxesBlocked: the 10 provided SVG files are not present — /tmp/svg-icons/ does not exist on this host, no *.svg anywhere in the repo or /tmp, and no attachments on this issue. Cannot embed verbatim per the spec without the files. Unblock by placing watch-on/off, star-on/off, fork, clone, notify-on/off, light-mode, dark-mode (+ the plus icon for #466) at /tmp/svg-icons/ or naming another path.
Icon file
watch-on.svg(verbatim from the user;1em,currentColor):Icon file
watch-off.svg(verbatim from the user;1em,currentColor):Icon file
star-on.svg(verbatim from the user;1em,currentColor):Icon file
star-off.svg(verbatim from the user;1em,currentColor):Icon file
fork.svg(verbatim from the user;1em,currentColor):Icon file
clone.svg(verbatim from the user;1em,currentColor):Icon file
notify-on.svg(verbatim from the user;1em,currentColor):Icon file
notify-off.svg(verbatim from the user;1em,currentColor):Icon file
light-mode.svg(verbatim from the user;1em,currentColor):Icon file
dark-mode.svg(verbatim from the user;1em,currentColor):Note: all 10 icon source files are posted as individual comments below (watch-on/off, star-on/off, fork, clone, notify-on/off, light-mode, dark-mode), each in an ```svg block — copy them verbatim into web/src/lib/icons.jsx. They are not all inline in the body; the comments are the source of truth for the exact paths.
Fix PR: #475 (branch fix/issue-465).
Asset-source note (resolves the earlier blocker): the 10 icon files were taken from the issue comments 4783-4792, mirrored to /tmp/svg-icons/ (read-only source, unmodified), and transcribed verbatim into web/src/lib/icons.jsx — each keeps its shipped viewBox (16/24/1024/1200) and currentColor paint (mechanically diffed token-for-token against /tmp/svg-icons/).
What landed: one shared Icon component consumed by all six controls with the issue's state mapping (watch/star/bell pairs, theme Show kept, fork/clone static); all old glyphs gone from web/src; #447 metrics + #463 spacing intact; no new deps; law-12 decision in docs/go/12_web_ui.md. Tests: node --test 998/996/2 (+10 net new, the 2 failures pre-existing live-server smoke, identical on main); vite + esbuild green, icons verified in the bundle. Browser proof open (shared-daemon loopback guard — no private daemon per workspace rules). Do NOT merge from my side.
Review of PR #475 (fix/issue-465, commit
fff3d37) against #465 — verified in scratch worktree /tmp/pr475 (removed afterward); main worktree untouched (still clean on main). No browser (per instructions — node tests + bundle reasoning only; browser proof remains open as the PR itself notes).(1) VERBATIM TRANSCRIPTION — PASS. Programmatically diffed all 12 d="..." path strings + all 10 viewBoxes in web/src/lib/icons.jsx against /tmp/svg-icons/*.svg: every path byte-identical, every viewBox preserved (16/16, 24/24 x3 incl. dark-mode, 1024 x5 incl. light-mode, 1200 fork). All paint via currentColor (12 occurrences); zero hex/rgb literals in code (the two scanner hits — "#465", "innerHTML" — are both in // comments). Clone's two paths wrapped in a JSX fragment — semantically identical. NOTE (non-blocking): the issue prose says light/dark-mode are both "viewBox 0 0 24 24", but shipped light-mode.svg is actually 0 0 1024 1024 (sun with fill-rule evenodd, kept correctly). Asset wins over prose — transcription is faithful to the source of truth. Also noted: both theme assets render sun-like as shipped; PR maps strictly by name (dark to dark-mode, light to light-mode) and documents this — correct call.
(2) SIX CONTROLS WIRED PER TABLE — PASS. Watch (Repo.jsx:215 Icon swaps on w().watching), Star (Repo.jsx:370 on s().viewer star state), Fork static icon left of count (Repo.jsx:662, icon before s().forks), Clone static left of label (Repo.jsx:100), bell on when unreadCount greater than 0 (NotificationTray.jsx:87), theme keeps existing Show when theme()==="dark" with dark-mode in branch / light-mode fallback (App.jsx:116-118). All import the one shared mechanism.
(3) OLD GLYPHS GONE — PASS. Fixed-string grep for the five old glyphs (eye-emoji, black-star, bell-emoji, sun, moon) plus the retired fork mark over web/src in the branch: zero hits.
(4) STATE DOUBLED — PASS. Watch/Star keep classList primary + aria-pressed alongside the icon swap; bell keeps badge + tray aria-label. Never color-only.
(5) #447 METRICS + #463 SPACING — PASS. btn px-2 py-1 text-sm classes byte-identical (icons added inside, no class edits); counts still LEFT of labels; .icon carries no margin (row gap-2 untouched). 390px re-based arithmetic (about 386px) re-verified in header-pills-447/header-gap-463/fork-pill-split-464 pins.
(6) BELL BADGE — PASS. Diff touches exactly one line (glyph to Icon); absolute overlay, aria-live polite, tray aria-label, outside-click/Esc all untouched.
(7) A11Y — PASS. aria-hidden="true" lives on the shared svg (covers all six, incl. theme where old spans carried it individually); toggle aria-pressed/aria-label/title texts unchanged. (Watch/Star glyphs were previously bare text with no aria-hidden — new state is strictly better.)
(8) .icon UTILITY — PASS (ui.css:82). inline-block 1em box, shrink-0 (flex safety), vertical-align -0.125em for non-flex contexts; .btn is inline-flex+gap so pills align via gap. No per-icon CSS; all six Icon usages are name-only (no class= overrides).
(9) NO RUNTIME FETCHES — PASS. No ?raw/innerHTML/dangerouslySetInnerHTML/fetch in icons.jsx (comment mention only); the one fetch( in Repo.jsx is the pre-existing setup.json call. Icons verified present in the vite bundle (all 12 path strings in dist/assets/*.js) with zero runtime requests.
(10) NO NEW DEPS; DOCS ACCURATE — PASS. package.json untouched (solid-js+router+marked+dompurify only, law 1 holds). Law-12 decision appended to docs/go/12_web_ui.md in the same commit; the entry's claims (counts, semantics, sun-like-asset note) all check out.
VERIFY RESULTS (scratch worktree, node_modules symlinked from main): node --test web/test/unit/*.test.js gives 998 total / 996 pass / 2 fail; the 2 failures are the pre-existing live-server smoke subtests, byte-identical on pristine main (same assertion pair, environment has no live Go server). vite build exit 0, .icon rule present in dist CSS, all icon data in dist JS bundle; esbuild exit 0.
Amusing footnote: my first attempt to post this comment with the literal old glyphs pasted in was rejected by the Forgejo MySQL backend (Error 1366 incorrect string value for 4-byte chars) — a live demonstration of the non-BMP footgun this PR removes.
MERGE RECOMMENDATION: ready to merge (browser proof still open per workspace loopback-guard constraints — same standing as #447/#450/#464 — but headless + bundle evidence is complete; no findings, nothing fixed, no push made).
Fixed by PR #475 (review clean — all 10 icons byte-identical to source, all 6 controls per table, #447/#463 intact), merged. Closing.