Adopt semantic color roles for the design system - #85
Conversation
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 foundation-contract defect
Reviewed head f3c9be6ce79decace5c384366710bd69d301ca2e against exact stacked base 0e4b4d875b0c6190df9dae535d5aebfc52a16cd7 (codex/design-system-adoption-a, PR #73), not main.
P2: Give the new invalid/warning boundary roles sufficient non-text contrast. The inline finding covers newly exported roles advertised as control/state boundaries. Their current values fall below the documented 3:1 target. There is no migrated product consumer of these roles in this snapshot, so this is a defective new foundation contract, not a claim that this PR already breaks an existing screen. Fix the role mappings and add focused pairing coverage; no component migration or palette-wide redesign is needed.
Non-blocking notes
- Reconcile the new semantic-only guidance with
DESIGN.md:154-258,339-340and the viewer'sColorPage.tsx:223-225,ColorTablePage.tsx:169-173, andOverviewPage.tsx:32-34, which still direct authors to use palette ramps. Those instructions now contradict the policy this PR introduces. - Bind warning/success/accent contrast checks to the new semantic fills as well as legacy palette consumers.
check-contrast.mjs:140-145still measures fixed palette pairs. In an isolated mutation, retargeting--affordance-warningfrom amber-3 to amber-9 left the contrast check green even though the dark warning-text pair fell to APCA Lc 15.4. Current unmutated text pairings pass; this coverage gap is not an additional current UI defect. The generic unresolved-reference skip predates this PR and is not attributed to it.
Validation and scope
- Independently ran the changed color and contrast guards and
git diff --checkat the reviewed, clean head. - Compiled the actual host and viewer CSS with pinned Tailwind/PostCSS and probed Chromium and WebKit: all 36 new semantic utilities resolve to their advertised variables across light → dark → light; all 186 pre-existing token properties have identical computed values versus the exact base in both modes and both CSS entry points.
- Independently checked border ratios with the repository's WCAG implementation. Complementary source review covered compatibility aliases, host appearance/backdrop ownership and contrast guards.
- Hosted CI run 35222499015 is successful and reports this exact head; broad CI-equivalent suites were not rerun locally. Browser probes are focused CSS evidence, not full-app, enlarged-text, layout, native packaging or assembled-stack acceptance.
- PR #73 remains open. This PR's body calls it a draft, but GitHub currently reports
isDraft=false; reconcile that readiness statement separately. No approval or merge performed.
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
…tings Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear at this head; no new blocking findings
Reviewed a8deea467f5d6c5700d61f7aa76fdce61814301a against exact stacked base 959aae7954002dcce0bdcb18eb2ef6dc20a13113 (PR #73), not main. This is a COMMENTED re-review, not approval or assembled-stack acceptance.
- The earlier danger/warning boundary blocker is resolved. Independent WCAG 2 arithmetic over the current literal values verifies all 16 documented pairings: both roles against base, panel, inset and popover in both modes. Minimum ratios are 3.228:1 for danger and 4.046:1 for warning, above the 3:1 requirement (
tokens.css:502–503,542–543;DESIGN.md:241–247). - The foundation contract is consistent. All 36 semantic roles have matching light/dark definitions, registry mappings and theme exports. Compatibility aliases retain their prior values; the host remains the appearance owner. Semantic-role guidance is aligned across the changed docs/viewer surfaces. The contrast mutation fixtures exercise the real checker and are included by root Vitest/CI.
- One optional guard clarification:
scripts/design-system/check-color.mjs:319–336says each role must declare both modes, but its dark lookup falls back to the light declaration. All current new roles explicitly declare both modes, so this is not a defect in the values under review. Clarify that inheritance is allowed, or distinguish the new-role set from inherited aliases if explicit declarations are intended to be enforced.
Evidence and limits
Two complementary source-only lanes completed on a verified bare Blox object store. Existing CI reports all 12 checks successful for this exact head. No repository code was checked out, installed, built, tested, imported or executed during this review; no new rendered-pixel or desktop acceptance is claimed. Boundary arithmetic was independently derived; APCA was not independently reimplemented.
The fixed-height settings-button finding on the stacked PR #73 base is inherited, not introduced by #85, and remains a separate stack concern. This review does not dismiss older reviews, approve, merge, or certify the later consumer-migration PRs.
Preserve main's mention enrollment, typography provenance and sizing/interaction repairs alongside the semantic color contract. Resolve the five overlapping files without restacking downstream branches. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. Resolved the five merge conflicts and pushed The integration preserves main’s mention enrollment/agent shapes, pinned typography provenance, #73 button sizing, and picker/signer repairs, alongside #85’s semantic colors. Both bounded independent source-review lanes are clear. Validation: 1,989/1,989 full Vitest tests, 43/43 design tests, 28/28 Chromium/WebKit viewer checks, app/viewer production builds, normal hooks and hosted DCO passed. Browser cases added/removed: 0/0. The PR description records exact snapshot and local port/setup details. Fresh CI is pending: https://github.com/block/buzz-app/actions/runs/35679190661. The existing changes-requested gate remains; no approval, dismissal, or merge was performed. Native/whole-app CI and desktop GUI acceptance are not claimed by these local checks. |
Buzz gets shared semantic colors for surfaces, text, borders and controls. Components choose a role by its purpose; the palette supplies light and dark values. Existing names remain as compatibility aliases while the later PRs migrate callers. Base UI remains the behavior layer, with public fonts and dependencies.
This is stack 1/4, targeting
mainafter #73 merged. #86 builds controls, #87 adds compositions, and #88 adopts them across the app. Only #85 was updated in this integration; #86–88 were not restacked.Current main integration (2026-09-22)
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Head
400118a0ba578321f9d2eb4c347057cfe052feeamerges main9fce7a73fcf644de089ff556b60e8ccd7f036863into previous heada8deea467f5d6c5700d61f7aa76fdce61814301a, without rewriting history. GitHub now reports MERGEABLE (no file conflicts), but the PR remains blocked by its existing changes-requested review and pending fresh CI.349089dc5b43339f8207930ef0a84808da6fd299. Hosted DCO Check passed at the pushed head.dist; neither setup failure reached test execution.Fresh hosted CI is still pending at handoff. Broad native/tool-integration and whole-app browser suites are left to CI; local viewer evidence is not desktop GUI or assembled-stack acceptance. No approval, review dismissal, merge, or downstream restacking was performed. Earlier validation below remains attributed to its historical snapshot.
Review fixes:
An inherited channel-menu test race also surfaced in CI. The test now waits for menu closure, reopened menu focus, and restored trigger focus before advancing. No timeout, product behavior, or assertion was weakened.
Historical validation at
27dd5d0:The normal hooks remain enabled. An earlier local run hit the existing five-second app composition timeout during a competing build; isolated and unchanged full-hook runs passed afterward. Native behavior is inherited unchanged from #73. Desktop GUI acceptance of the assembled stack remains separate. Existing requested-change reviews need reviewer re-review; no review was dismissed.
Previous base refresh:
a8deea467f5d6c5700d61f7aa76fdce61814301amerges #73959aae7, including current main’s presence, development notification pause and Dock unread indicator. This merge was conflict-free and adds no new foundation behavior. Normal hooks passed app/viewer typechecks, design guards and all 1,865 unit tests in 180 files (17.81s wall, 107.50s summed execution). DCO and diff checks pass. All 12 hosted checks pass at this new head, including browser/native checks, measurements, security and DCO. The earlier local browser evidence above remains attributed to its stated snapshot.