crueber/walhub

Watch

Fork 0

Design Fix for http://192.168.2.48:8080/crueber/walhub/issues/1 #31

New issue

Closed

opened 2026-09-04 16:22:57 +00:00 by crueber · 4 comments

crueber commented

2026-09-04 16:22:57 +00:00

Owner

Icons should be icons, not words.

When reacting, it should show the icons (and a number) on the comment that it is reacting to.

![image](/attachments/fdd9c482-71f1-4d7d-8be8-9c5c6ba635bb) Icons should be icons, not words. When reacting, it should show the icons (and a number) on the comment that it is reacting to.

image.png

78 KiB

crueber commented

2026-09-04 16:31:48 +00:00

Author

Owner

Starting work on this: emoji glyphs for the reaction picker (with word-form aria-labels/titles) + a per-comment emoji+count summary row. Working on branch fix/issue-31.

crueber referenced this issue from a commit

2026-09-04 17:02:32 +00:00

Fix #31: emoji reaction icons + per-comment summary row

crueber referenced this issue from a pull request that will close it,

2026-09-04 17:02:36 +00:00

Fix #31: emoji reaction icons + per-comment summary #40

crueber referenced this issue

2026-09-04 17:02:47 +00:00

Post-mutation refetch can commit SWR-stale bodies (reaction live update flake) #41

crueber commented

2026-09-04 17:02:51 +00:00

Author

Owner

Fixed by #40 (branch fix/issue-31, not merged): picker renders emoji glyphs with word-form aria-labels/titles, each comment shows its emoji+count summary row from reaction_summary with toggle chips, reaction_changed rows read e.g. reacted eyes on #0. Verified: node --test 190 pass, real-browser dark+light with zero console errors. One residual, tracked as #41: post-mutation refetch can commit SWR-stale bodies on disk-starved hosts.

Fixed by https://git.packden.us/crueber/walhub/pulls/40 (branch fix/issue-31, not merged): picker renders emoji glyphs with word-form aria-labels/titles, each comment shows its emoji+count summary row from reaction_summary with toggle chips, reaction_changed rows read e.g. reacted eyes on #0. Verified: node --test 190 pass, real-browser dark+light with zero console errors. One residual, tracked as #41: post-mutation refetch can commit SWR-stale bodies on disk-starved hosts.

crueber commented

2026-09-04 17:04:29 +00:00

Author

Owner

PR #40 review (fix/issue-31 @ 07d12e0, verified in scratch worktree /tmp/pr40):

PASS

Emoji allowlist exact: REACTIONS (web/src/lib/reactions.js:8) == server ReactionContents (internal/issues/model.go:53-56): +1, -1, laugh, hooray, confused, heart, rocket, eyes. No invented codes, no 400 path (UI only sends from the list; unknown pass-through at reactions.js:26-28 is display-only).
Summary is a pure view of thread.reaction_summary (Issue.jsx:77,177 via summaryEntries) — no N+1 fetches.
Toggle sane: remove-first, 404-falls-back-to-add, single reload, reports only on double failure.
No new deps (package.json untouched), no TS, .chip classes carry dark/light already.
ThreadTimeline summaryFor is optional (ThreadTimeline.jsx:37 props.summaryFor?.(ev)); Pull.jsx:644 passes neither actionsFor nor summaryFor — PR timelines unaffected.
Doc decision (docs/features/02_issues.md) faithfully describes glyphs, aria, summary row, toggle, reaction_changed text.

FOUND + FIXED (ce6367a, pushed to origin/fix/issue-31)

Issue.jsx:180: summary row was <p aria-label=...> — aria-label on a role-less
is ignored by assistive tech, so the group label never announced. Changed to <div role="group" aria-label=...>, matching the actionsFor row's role="group" pattern. Glyph spans stay aria-hidden, wire word kept in label/title.

VERIFY

reactions.test.js 6/6 pass; full node --test web/test/unit/*.test.js 190/190 pass (pre- and post-fix); vite build clean post-fix.

MERGE RECOMMENDATION: ready to merge.

PR #40 review (fix/issue-31 @ 07d12e0, verified in scratch worktree /tmp/pr40): PASS - Emoji allowlist exact: REACTIONS (web/src/lib/reactions.js:8) == server ReactionContents (internal/issues/model.go:53-56): +1, -1, laugh, hooray, confused, heart, rocket, eyes. No invented codes, no 400 path (UI only sends from the list; unknown pass-through at reactions.js:26-28 is display-only). - Summary is a pure view of thread.reaction_summary (Issue.jsx:77,177 via summaryEntries) — no N+1 fetches. - Toggle sane: remove-first, 404-falls-back-to-add, single reload, reports only on double failure. - No new deps (package.json untouched), no TS, .chip classes carry dark/light already. - ThreadTimeline summaryFor is optional (ThreadTimeline.jsx:37 `props.summaryFor?.(ev)`); Pull.jsx:644 passes neither actionsFor nor summaryFor — PR timelines unaffected. - Doc decision (docs/features/02_issues.md) faithfully describes glyphs, aria, summary row, toggle, reaction_changed text. FOUND + FIXED (ce6367a, pushed to origin/fix/issue-31) - Issue.jsx:180: summary row was `<p aria-label=...>` — aria-label on a role-less <p> is ignored by assistive tech, so the group label never announced. Changed to `<div role="group" aria-label=...>`, matching the actionsFor row's role="group" pattern. Glyph spans stay aria-hidden, wire word kept in label/title. VERIFY - reactions.test.js 6/6 pass; full `node --test web/test/unit/*.test.js` 190/190 pass (pre- and post-fix); `vite build` clean post-fix. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-04 17:04:41 +00:00

crueber commented

2026-09-04 17:04:42 +00:00

Author

Owner

Fixed by PR #40 (review: a11y role fix on top; 190/190 node tests), merged. Closing.

crueber referenced this issue from a commit

2026-09-04 17:04:42 +00:00

Merge pull request 'Fix #31: emoji reaction icons + per-comment summary' (#40) from fix/issue-31 into main

crueber referenced this issue

2026-09-04 17:09:39 +00:00