feat(#385): replace the chrome emoji with the icon set - #405
Merged
Conversation
18 of 20 emoji across the seven surfaces outside the page children. Seven new icons drawn to the grid in icons.ts: cookie, copy, pin, ban, tip, eye, eye-off. The grid test validated all seven unprompted; I rendered them anyway, because it cannot see whether a drawing reads as the thing it names -- which is how `settings` and `theme` were caught in #377. Every replacement is DECORATIVE, and that is a per-site judgement rather than a default. In each case the accessible name already existed beside the glyph: CookieConsent aria-label="Cookie" next to "We use cookies to enhance your experience" -- the name repeated the copy A11yDevOverlay button already labelled "Accessibility violations: N" and "Collapse accessibility panel" LocationButton text reads "Location blocked" / "Update location" SEOAnalysisPanel <h4>Strengths (N)</h4> / <h4>Weaknesses (N)</h4> above the lists, so the marker was never the only non-colour signal schedule "Prepare for the meeting:" follows messages/setup button already labelled "Show/Hide password" -- and the two eye emoji were near-identical at button size TWO ARE DELIBERATELY LEFT, because they are text and not chrome: A11yDevOverlay:252 🎉 inside the sentence "No accessibility violations" SEOAnalysisPanel:42 ✗ in CLIPBOARD EXPORT text, where an SVG cannot go BlogContent needed different handling. Its copy button is built as an HTML string with an inline onclick, so <Icon> cannot be used; an `iconSvg()` helper emits markup with single-quoted attributes, because the surrounding onclick is a double-quoted HTML attribute and double quotes would terminate it. The button also swaps two pre-rendered spans by `display` rather than rewriting innerHTML -- injecting SVG through an attribute-embedded JS string is exactly where quoting breaks. That change surfaced a real defect: with the emoji gone the button had NO text content, leaving `title` as its only accessible name. It now carries sr-only "Copy code". The test asserted toHaveTextContent('📋'); it now asserts the real label, which is a stronger check than the glyph was. Also refreshed two doc comments in Icon.tsx and its a11y spec that cited CookieConsent's aria-label as a live example -- it no longer is. 35 E2E green; 239 unit green Refs #385 Part of epic #376 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate was very nearly inert. Its selector matched 60 elements at 390px and exactly ONE was visible -- the hamburger itself. Everything else sat inside a closed dropdown, isVisible() skipped it, and the test reported green having checked a single 44px button. It had been that way for as long as the mobile nav has been a dropdown. #378 flagged this as something its own nav regrouping WOULD cause. It had already happened. Opening the menu found a real defect on the first run: **Sign Out at 144x26px**, 18px under the standard, in both the desktop and mobile menus. Neither had a className at all, so both inherited DaisyUI's 26px menu default. Never measured, never reported. Fixed with min-h-11. Two changes to the gate: click(), not focus() -- a DaisyUI dropdown is held open by :focus-within and a programmatic focus on the <label> does not reliably establish it (measured: focus 3 targets, click 6). A COVERAGE FLOOR. Every check is conditional on isVisible(), so zero measured is indistinguishable from zero failures. The floor makes a drop visible instead of silent. 6 is measured, not chosen, and the comment is explicit about what it does NOT mean: the selector deliberately exempts inline text links, so the menu's <a> items are out of scope by design. 6 is full coverage for this selector, not proof every nav target is checked. MUTATION: leave the menu closed -> "Only 5 nav touch targets were measured, down from 6", naming the cause. This is the prerequisite that makes #378 safe to attempt: regrouping the nav into `Demos ▾` and `Display ▾` now has a gate that notices if targets get hidden rather than fixed. Refs #378, #396 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TortoiseWolfe
added a commit
that referenced
this pull request
Jul 30, 2026
…nd a correction to my own justification (#464) * feat(#431): the messaging surface joins the depth vocabulary, and /80 turns out to fail AAA Presentational only. No changes to key handling, RLS, or the authorization contract, and the MessageThread scroll container is untouched - `parentRef` at :369 is `absolute inset-0 overflow-y-auto` with `data-testid="message-thread"`, and four sites in tests/e2e/messaging/ dispatch scroll events against it because WebKit does not reliably fire them for programmatic scrollTop. Restyling it would have put that at risk for no visual gain. Two tab strips become rails - ConversationList's All/Unread/Archived and UnifiedSidebar's Chats/Connections. `.tab` gets no depth from #427, and DaisyUI's `tabs-boxed` and `tabs-bordered` were a fifth vocabulary alongside the rails already used by GlobalNav, /blog, /docs, /payment and the admin console. Two hand-rolled modals - ReAuthModal ("Enter Your Messaging Password", which is what /messages actually shows a returning user) and ConsentModal - carried `shadow-xl` as their only elevation. They are not `.modal-box`, so #427's plate rule never reached them, and simply deleting the shadow would have left them flat. They now carry `sh-plate` explicitly. CHECKED WHAT ASSERTS ON THESE TABS BEFORE TOUCHING THEM, AND MY FIRST CHECK WAS INCOMPLETE. I grepped tests/e2e and found `toHaveClass(/tab-active/)` belonging to ConnectionManager (friend-requests.spec.ts:307,310) and to PaymentButton's stripe/paypal tabs (three payment specs) - both left alone, deliberately. Then the vitest run failed: UnifiedSidebar.test.tsx:70 asserts `tab-active` too. Colocated unit tests are as much a consumer of these classes as the E2E suite is, and grepping only tests/e2e missed them. The assertion is updated to `sh-rail-active` WITH a negative case on the inactive tab - as written it would have passed on a component that marked every tab active - and a note that `aria-selected` on the line below is the real contract, since a class being present says nothing about whether it renders (#430). /80 AND /85 ARE NOT SAFE, AND NOW THERE ARE NUMBERS. #411 measured /60 and /70 failing and removed them; /80 and /85 were left as presumed safe. Measured properly by canvas readback, compositing the dimmed foreground over each surface, on scripthammer-light: /100 /85 /80 /70 base-100 12.96 8.30 7.08 5.20 base-200 11.73 7.73 6.62 4.98 base-300 10.10 6.88 6.05 4.57 /80 on base-200 is 6.62:1, under the 7:1 gate - and MessagingGate's card is `bg-base-200` with dimmed text inside it. /85 on base-300 is 6.88:1. The dark theme is comfortable everywhere except /70 on base-300. Full tables in #462. So the nine dim levels removed here were a real AAA failure, not just an unmeasured risk. ConversationListItem:205 keeps its `/90` deliberately: it is the read/unread distinction, it measures ~9:1 on base-100 and ~8.6:1 on base-200, and it passes. Removing it would have cost meaning for nothing. VERIFIED AGAINST THE STATE THAT SHOWS THE PAGE. /messages/setup redirects to /messages for any user who already has messaging keys, which the standing test user does - so the first run measured /messages twice and reported /messages/setup clean without ever loading it. The same shape as #454. Created a throwaway keyless user, minted a session for it by service-role grant, measured the real page (h1 "Set Up Encrypted Messaging", redirects=0), then deleted the user. 3 routes x 2 house themes: contrast fails 0, horizontal overflow 0 at 320/390/428/768/1024/1280/1440, shadow-xl 0 on the messaging surface. 82 unit tests pass across 10 files, type-check clean. Closes #431. Refs #462. * docs(#463): three comments justified a clamp that was already fixed AdminGate.tsx:105, game/page.tsx:25 and schedule/page.tsx:20 each said, in the present tense, that Tailwind's `container` clamps to the previous registered breakpoint and throws away up to 43.9% of the viewport at 767px, citing #373 — as the reason those call sites had been given an explicit measure instead. None of it is true of this stylesheet, and it was not true when those comments were written. #373's §A1 was fixed by the `@utility container` override in globals.css:56-59. Measured live on /blog: viewport container max-width rendered margin-left 390px 1280px 390 0px 428px 1280px 428 0px 767px 1280px 767 0px 1023px 1280px 1023 0px 1440px 1280px 1280 80px Full width at every tier below 1280, nothing wasted. #373's table describes the state before its own fix landed. The code is unchanged — the explicit measures are fine, they are just a readability choice rather than a bug fix, and the comments now say so. Verified that removing `container`'s cap did not matter above 1280px either: /account and /profile hold a 672px box with gutters 384/384, 624/624 and 944/944 at 1440/1920/2560, /contact and /accessibility hold 896px, and horizontal overflow is zero throughout. globals.css:40 keeps the 43.9% figure on purpose: it sits inside the `@utility container` block documenting why the override exists. Historical context for a fix is not a stale claim. Why it happened, which is the part worth keeping: I read #373's headline and its measurement table and skipped its own resolution note, which is the FIRST LINE of the body. That ticket is a mix of fixed and open sections — §A1 fixed, §A2 onward open — and I quoted a figure from the fixed part as if it were current. A ticket that documents its own partial resolution has to be read to the end before any number in it is quoted. Folded into this branch rather than sent as its own PR because the E2E concurrency group allows one waiting run (#444), and an hour of queue for a three-comment correction is not a good trade. Closes #463. * fix(#385): drop the last decorative emoji from the a11y overlay, and un-dim its empty state `A11yDevOverlay.tsx:252` read "No accessibility violations 🎉". The emoji is gone — it cannot take `currentColor` and renders differently on every OS, which #385 lists as a real problem regardless of which direction that ticket settles on. The sentence carries the meaning without it. The same line carried `text-base-content/60`, which measures 5.20:1 on base-100 in scripthammer-light — under the 7:1 gate. #411 removed /60 and /70 elsewhere; this one survived because the overlay is dev chrome that no route sweep visits. Full per-surface opacity table is in #462. Scope kept deliberately narrow. #385 is BLOCKED on an owner decision about whether design-specified surfaces move from icons to typographic marks, and its body now records that plus the measured state: five of its seven surfaces already carry no decorative glyph after PR #405. `SEOAnalysisPanel.tsx:42` keeps its `✗` on purpose — that string is built for the CLIPBOARD, not for markup, and a plain-text marker is the right instrument in text a user pastes into a document. The JSX comment for this change went in as a child of the `<ul>`, not inside the ternary branch. Placed between `? (` and its element it is a second root node in a parenthesised expression, which is a syntax error — the same trap recorded in #429's commit, walked into again and caught by type-check. Refs #385. Refs #462. --------- Co-authored-by: TurtleWolfe <TurtleWolfe@users.noreply.github.com>
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
2 participants
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.
Summary
18 of 20 emoji replaced with the icon set, across the seven surfaces outside the page children. Seven new icons:
cookie,copy,pin,ban,tip,eye,eye-off.The grid test in
icons.tsvalidated all seven unprompted — which is what documenting the grid bought. I rendered them anyway, because that test cannot see whether a drawing reads as the thing it names; that is exactly howsettingsandthemewere caught in #377.Every replacement is decorative — decided per site, not defaulted
In each case an accessible name already sat beside the glyph, so adding a second one would repeat rather than inform:
CookieConsentaria-label="Cookie"next to "We use cookies to enhance your experience" — it announced "Cookie, We use cookies…"A11yDevOverlayLocationButtonSEOAnalysisPanel<h4>Strengths (N)</h4>and<h4>Weaknesses (N)</h4>above the lists — the ✓/✗ was never the only non-colour signalschedulemessages/setupTwo are deliberately left, because they are text and not chrome
An SVG cannot go in clipboard text, and the 🎉 is prose. Replacing either would be swapping an icon system into a sentence.
BlogContentneeded different handlingIts copy button is built as an HTML string with an inline
onclick, not JSX, so<Icon>cannot be used. AniconSvg()helper emits the markup with single-quoted attributes — the surroundingonclickis a double-quoted HTML attribute and double quotes would terminate it. The button also swaps two pre-rendered spans bydisplayrather than rewritinginnerHTML, because injecting SVG through an attribute-embedded JS string is precisely where quoting breaks.That surfaced a real defect. With the emoji gone the button had no text content at all, leaving
titleas its only accessible name. It now carries sr-only"Copy code".The existing test asserted
toHaveTextContent('📋'). I fixed the button rather than the assertion — it now asserts the real label, which is a stronger check than the glyph was.Also
Refreshed two doc comments in
Icon.tsxand its a11y spec that citedCookieConsent'saria-labelas a live example. It no longer is — same body-is-truth discipline (#358) applied to code comments.Testing
accessibility,depth-tokens,container-width,blog-touch-targets,color-contrastAAAtype-check,lintcleanRelated Issues
Refs #385
Part of epic #376
🤖 Generated with Claude Code