Skip to content

Review 5426

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 2 revisions

#5426 — feat(CheckboxInput): expose theme targets for the control and the marks

freddymeta (contributor; head on facebook/astryx) · MERGED 7b60efafa73 · view on GitHub

Verdict: approve, merged

Head reviewed: b954eb6f49ed1952539787e6abd8420231b73532, unmoved between draft and post. No prior reviews; the only issue comments were vercel[bot] and the PR Analysis Report.

Reviewed as one of a SET of four — see Review-5418, Review-5420, Review-5183 and the set-level findings below. All four are rows of one author's own RFC, #5417, so R6d's "review them together" applied even though they are not competitors.

Problem

A theme that wants a heavier checkmark or a differently-proportioned indeterminate bar has to reach them with .astryx-checkbox-indicator > svg and > span — element-and-order selectors that keep parsing and silently stop matching on any restructure. The radio's equivalent mark has had a target since astryx-radio-indicator-dot; the checkbox's two have not. Demonstrated, not asserted: #5417 quotes the live EPS rules, which want a 12px glyph where Astryx paints 10.

Solution (1 decision · ~15 runtime lines of 23)

1. the checkmark <svg> and the indeterminate <span> get themeProps,
   both reflecting size — CheckboxIndicator.tsx:231, :249

The title and body describe a PR twice this size — four targets across CheckboxInput and Indicator, where the diff ships two on Indicator alone, and a test plan whose negative control renames astryx-checkbox-input-control, which is not in the diff. The changeset is accurate. That is the finding in the posted review, and it is one instance of a set-wide pattern (see below).

Impact

End users of any app whose theme opts in: the checkmark and the indeterminate bar become the theme's shape rather than Astryx's — the one place in a checkbox a design system usually has an opinion. Nothing changes for anyone until a theme names the keys.

Builders: the targets live on CheckboxIndicator, so DropdownMenuCheckboxItem and every other host of the shared indicator picks them up at once — a theme writes one key and every checkbox in the system follows, which is what radio-indicator-dot already does.

These work in a production build. #5410 merged the same morning (c47e75495d5), before this did, so the draft's "inert in a built app until #5410 lands" caveat never applied to anything shipped. Had the order gone the other way, both keys would have rendered in dev and done nothing in a build — the same failure as the 58 shipped theme keys #5410 found.

API

no API change.

Theme targets

+ astryx-checkbox-indicator-check → the checkmark <svg>, reflecting size      (public, permanent)
+ astryx-checkbox-indicator-dash  → the indeterminate <span>, reflecting size (public, permanent)

Verified rendered in Chromium: <svg data-size="md"> on core-checkboxinput--checked, <span data-size="md"> on --indeterminate.

Ossification

Class, named, with a merged member in the same file. astryx-radio-indicator-dot is the mark inside astryx-radio-indicator, shipped and documented at Indicator.doc.mjs:142, six lines below the two additions. <root-target>-<part> holds without exception across the 238 themeProps() call sites on main.

What already exists and why it does not reach: astryx-checkbox-indicator is the box, not the mark, and sizing the box does not size the glyph — checkmarkSizeStyles sets width/height atomics on the <svg> directly (CheckboxIndicator.tsx:137). useIndicator can replace the component wholesale, which the RFC considers and rejects for a size change: you would reimplement checked / indeterminate / disabled to change 10px to 12px.

Breaking

  • API — no.
  • Visual — no. The stylex.props arguments are unchanged, only wrapped in mergeProps; measured glyph box 14×14 at md before and after; nothing in the repo's CSS matches the new names.
  • Theme — no. astryx-checkbox-indicator and its legacy alias astryx-checkbox (deprecatedFor: 'checkbox-indicator') untouched. Indicator.doc.mjs has docs and docsDense and no docsZh, and docsDense carries no theming block, so the single doc hunk is complete — the failure mode this PR could plausibly have had and does not.

Performance & resources

Two mergeProps calls per rendered indicator replacing two bare spreads. No effect, no listener, no observer, no layout read. Indicators come in crowds — a CheckboxList or DropdownMenu mounts one per row — so this is per-row work: two object constructions and two string concatenations, in a component that already calls themeProps once on the box. Not measured, and not called negligible; the diff changes no order of growth, only a constant factor on a call that already happens.

Visual evidence

None, and the reason is a real one: nothing rendered changes. Checked rather than assumed — the four stylex.props arguments on the <svg> and the <span> are identical before and after, the measured glyph box is unchanged, and no existing selector matches either name.

The measurement that mattered was cascade reachability, run in real Chromium against all four heads merged onto ea888ef:

✓ reaches  astryx-checkbox-indicator-check  width  14px → 22px  <svg>  data-size=md
✓ reaches  astryx-checkbox-indicator-dash   width  12px → 22px  <span> data-size=md

Probe banked at {review-root}/probe-kit/theme-target-reachability.cjs (SB_PORT=6610 node …). It injects the exact shape Theme.tsx emits — @layer astryx-theme { .<class> { … } } — and reads the computed value back.

Risk class

Low-risk on three of R19's four, and not on the first: it adds permanent public surface. No behavior change, no performance regression, nothing grew.

Judgement

approve, and merge.

1. [not blocking] the title and body describe four targets on two components;
   the diff ships two on one
   → the changelog entry reads correctly, but anyone who read the PR looks for
     `astryx-checkbox-input-control` and finds nothing — and the test plan
     cites a negative control on that name
                                   · PR title; body "Change" table; test plan

Two things checked and found fine, worth recording because they are where this shape usually goes wrong:

  • The focus-ring hazard the author documents did not recur. His body records a first attempt folding {...focusProps} into mergeProps and breaking three WCAG 2.4.7 assertions. Neither element here carries focusProps; both are aria-hidden decorative marks inside a box that owns the ring.
  • Naming follows the siblings rather than an invented scheme (R3k): radio-indicator-dot is the precedent, and <root-target>-<part> holds across all 238 targets on main.

The review, as posted

Thanks — this is clean, and mirroring radio-indicator-dot is right. The title and body still describe the CheckboxInput slot and native input; neither is in the diff.

[Reviewed by Robohands]

27 words. No code block, no closing question (R12 approve rules); no Discord link on an approve (R12e).

What changed before posting

Nothing. Cindy read the presentation in full and ruled "seems okay approve, next"; posted verbatim.

Merge

review-required stayed pending for ~30s after the approve — the Review signal workflow runs on the pull_request_review event and has to flip it. This PR's head is on facebook/astryx, so the run was queued, not action_required, and cleared itself; on a fork head it would have needed a /rerun (R21's second-order trap). Merged by squash once mergeStateStatus went CLEAN. Auto-merge was never armed.

Set-level findings

Three things belong to the set, not to this PR, and are recorded here because this is the first of the four to land.

S1 — #5417 has never been answered, and reviewing its rows one at a time answers it by accretion. The RFC is a real audit — 55 EPS CSS files, 273 selector-rules classified against 243 documented targets, 46 on elements with no class at all — and it asks for a per-row judgement. It has zero comments. Twelve rows remain. R28: escalate the category, not the instance. Open on Rulings.

S2 — nothing checks that a theme rule REACHES a target. themingTargets.test.ts checks the class is documented; #5334's eslint rule checks it is not clobbered; #5410's theme-var-reachability.js checks a documented public var is settable, and skips anything private/--_-prefixed (theming-targets.mjs:161), so no target class is covered. All nine targets across the four PRs reach on the dev path, measured. Whether the guard should exist is genuinely open — nine of nine passing is also an argument that it would only ever be green.

S3 — the bodies describe PRs about twice the size of the diffs. #5426 and #5418 both promise four targets and ship two; #5420 and #5183 are accurate. The changesets are right in every case, which is the tell that the trim was deliberate and the bodies were not revisited. Said once, on this PR.

Not verified

  • The built path for these targets specifically. #5410's own gate 2 establishes the mechanism; not re-derived here.
  • A full pnpm test. Ran themingTargets.test.ts (302 assertions, green on the four-way merged tree); CI green with 17 successes.

Clone this wiki locally