Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
|
AI review status for this pull request.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering-owner decision about the component:CheckboxInput change this pull request carries, plus two fixes to that record change.
Owner decision: FR11 and AV1 are rewritten so a first-party indicator can paint its own press while CheckboxInput turns its rounded-square pressed layer off over it; the current AV1 says CheckboxInput "still paints focus and pressed feedback on the resolved root". An engineering owner records approval by commenting /approve-spec with the full head commit SHA, and the author may do so as an engineering owner.
Blocking:
- Add a
## Writing rubric resultssection to the description, since the pull request changes a knowledge record (the one in #9716 covers the same edit). - Update the CheckboxInput record's contract-at-a-glance Behavior row, which still says CheckboxInput owns the pressed overlay, and state the new exception once in AV1 with FR11 citing it instead of repeating the sentence.
Without the circle mapped, CheckboxInput and CheckboxList render exactly as before.
[Automated review]
| ### Allowed variation | ||
|
|
||
| - **AV1 — Indicator replacement.** A theme may replace the named checkbox indicator if it preserves the Indicator prop and decorative ownership contracts; CheckboxInput still paints focus and pressed feedback on the resolved root. | ||
| - **AV1 — Indicator replacement.** A theme may replace the named checkbox indicator if it preserves the Indicator prop and decorative ownership contracts; CheckboxInput still paints focus and pressed feedback on the resolved root. A first-party replacement indicator may paint its own press from the row's hover and press (an internal marker; not public API), while CheckboxInput still paints focus. |
There was a problem hiding this comment.
This sentence repeats FR11 word for word, and the contract-at-a-glance Behavior row still says CheckboxInput "owns its focus ring and pressed overlay". Suggest stating the exception once here, having FR11 cite AV1, and amending the Behavior row.
| // under Reduce Motion its change is instant. Both keys are spelled as | ||
| // Core spells them, because Core exports neither. |
There was a problem hiding this comment.
Non-blocking (INV29): the stated reason is not the constraint; Core keeps these keys literal because StyleX ranks a constant media key in a later layer. Suggest: "StyleX ranks a constant media key above a literal one. So both keys stay literal, as in Core."
| // that paints its own press. Only this row declares it, so a mapped | ||
| // indicator in a row that paints its own press paints nothing twice. |
There was a problem hiding this comment.
Non-blocking (INV29): hard to parse. Suggest: "Only this row declares it. A menu row paints its own press, so an indicator mapped there paints no second press."
e83596c to
752d611
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision about the component:CheckboxInput change this pull request carries, plus three fixes.
Owner decision: FR11 and AV1 are rewritten so a first-party indicator can paint its own press while CheckboxInput turns its rounded-square pressed layer off over it; the current AV1 says CheckboxInput "still paints focus and pressed feedback on the resolved root". An engineering owner records approval by commenting /approve-spec with the full head commit SHA, and the author may do so as an engineering owner.
Blocking:
- Under a finger, holding a mapped CheckboxList row on its label paints the row's press and a second press on the circle, which the built-in box and a mouse do not, so declare
--_indicator-press-imageonly under the row's owndata-astryx-pressstates and hold the checkbox, not the row, in the browser case that asserts the press. - Add a
## Writing rubric resultssection to the description, since the pull request changes a knowledge record. - Update the CheckboxInput record's contract-at-a-glance Behavior row and ownership bullet, which still say CheckboxInput owns the pressed overlay, and state the new exception once in AV1 with FR11 citing it instead of repeating the sentence.
Without the circle mapped, CheckboxInput and CheckboxList render and press exactly as before.
[Automated review]
| // that paints its own press. Only this row declares it. A menu item paints | ||
| // its own press, so a mapped indicator there paints none. | ||
| indicatorPressImage: { | ||
| '--_indicator-press-image': 'var(--_press-paint-image)', |
There was a problem hiding this comment.
Under a finger this hands down a press whenever any ancestor is pressed: --_press-paint-image follows --astryx-press-alpha, which this row inherits from a pressed CheckboxList li. With the circle mapped, holding a row's label paints the row's press and a second one on the circle, while the built-in box (whose layer is gated on this row's own data-astryx-press) and a mouse stay unpressed. Declaring it only under the row's own press states keeps the circle in step with the host layer, for example:
indicatorPressImage: {
'--_indicator-press-image': {
default: 'none',
'[data-astryx-press="on"]': 'var(--_press-paint-image)',
'[data-astryx-press="fading"]': 'var(--_press-paint-image)',
},
},| page, | ||
| }) => { | ||
| await openStory(page, storybook(), STORY); | ||
| const at = await centreOf(page, 'Mountains'); |
There was a problem hiding this comment.
This holds the row's centre, which is the ListItem label, not the checkbox, so the case asserts the double press described on CheckboxInput.tsx:87. Holding the circle (circleCentreOf) tests the circle's own press; a second case holding the label could assert that the circle stays unpressed.
| ### Allowed variation | ||
|
|
||
| - **AV1 — Indicator replacement.** A theme may replace the named checkbox indicator if it preserves the Indicator prop and decorative ownership contracts; CheckboxInput still paints focus and pressed feedback on the resolved root. | ||
| - **AV1 — Indicator replacement.** A theme may replace the named checkbox indicator if it preserves the Indicator prop and decorative ownership contracts; CheckboxInput still paints focus and pressed feedback on the resolved root. A first-party replacement indicator may paint its own press from the row's hover and press (an internal marker; not public API), while CheckboxInput still paints focus. |
There was a problem hiding this comment.
This sentence repeats FR11 word for word, and the first sentence of AV1, the contract-at-a-glance Behavior row (line 55) and the ownership bullet (line 82) still say CheckboxInput paints or owns the pressed overlay. Suggest stating the exception once here, having FR11 cite AV1, and amending the Behavior row and the ownership bullet.
| // `:active` fill, and the image would paint a second press. The key | ||
| // stays literal, because StyleX ranks an imported at-rule key | ||
| // differently. |
There was a problem hiding this comment.
Non-blocking (INV29): "ranks … differently" does not give the constraint; Core states it in motion/reducedMotion.stylex.ts. Suggest: "StyleX puts a constant media key in a later priority layer than a literal one. So the key stays literal."
752d611 to
78eda2a
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision about the component:CheckboxInput change this pull request carries; nothing else blocks.
Owner decision: FR11 and AV1 are rewritten so a first-party indicator can paint its own press while CheckboxInput turns its rounded-square pressed layer off over it; the current AV1 says CheckboxInput "still paints focus and pressed feedback on the resolved root". An engineering owner records approval by commenting /approve-spec with the full head commit SHA, and the author may do so as an engineering owner; if the owners decline, the Core change and the record edit come out and the circle keeps CheckboxInput's square press.
Holding a CheckboxList label under a finger now presses only the row, the description reports the writing-rubric results, and the record states the exception once in AV1. Without the circle mapped, CheckboxInput and CheckboxList render and press exactly as before. Two optional comment rewrites are left inline.
[Automated review]
| // that paints its own press, but only in its own press states. A pressed | ||
| // ancestor row reaches it no more than it reaches the built-in box. The |
There was a problem hiding this comment.
Non-blocking (comment style): "A pressed ancestor row reaches it no more than it reaches the built-in box" is hard to parse. Suggest: "A pressed outer row hands down nothing, as for the built-in box."
| // The theme's duration on every pointer and none under Reduce Motion, | ||
| // as Core's indicators play their marks. |
There was a problem hiding this comment.
Non-blocking (comment style): this restates the declaration below it. Suggest deleting it, or keeping only "Core's indicators time their marks the same way."
78eda2a to
bb20ec6
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision about the component:CheckboxInput change this pull request carries; nothing else blocks.
Owner decision: FR11 and AV1 are rewritten so a first-party indicator can paint its own press while CheckboxInput turns its rounded-square pressed layer off over it; the current AV1 says CheckboxInput "still paints focus and pressed feedback on the resolved root". The existing record has to be updated for this intentional change: an engineering owner records approval by commenting /approve-spec with the full head commit SHA, and the author may do so as an engineering owner. If the owners decline, the Core change and the record edit come out and the circle keeps CheckboxInput's square press.
Holding a CheckboxList label under a finger presses only the row, the description reports the writing-rubric results, and without the circle mapped CheckboxInput and CheckboxList render and press exactly as before. Optional: the comments at CheckboxInput.tsx lines 89–90 and circleCheck.stylex.ts lines 52–53 could be tightened.
[Automated review]
iOS marks multiple selection with a circle: a table row's edit-mode control and a list's `.multiselect()` accessory, an empty ring that fills with a white checkmark. Astryx's only multiSelection indicator is the checkbox box, and rounding it gives neither the clear ring nor UIKit's checkmark. CircleCheckIndicator is a multiSelection-family indicator in Lab. A theme maps it to `checkbox`, and every CheckboxInput and CheckboxList draws it, keeping its checkbox semantics and focus ring. Its geometry is UIKit's, measured on iOS 26.2 at two text sizes and kept beside it; its colours are the theme's. Under a finger the mark changes in one frame; a mouse fades over the theme duration, and Reduce Motion makes that change instant. It paints its own round press: hover and a mouse press come through the row's scope marker, and a touch press from the image that only a CheckboxInput row hands down, so a mapped menu checkbox item paints its press once. In forced colors a system-coloured border draws the ring. It comes with a story, a CLI discover entry and a registry route.
bb20ec6 to
df21f7c
Compare
github-actions
Bot
removed
needs:design-review
There was a problem hiding this comment.
Recommending approval: CircleCheckIndicator follows the current Indicator replacement rule and CheckboxInput's exception for an indicator that paints its own press, and nothing blocks.
At this head a held mouse or finger presses the circle itself while CheckboxInput's square layer stays off, holding a CheckboxList label presses only the row, and focus, forced colors and Reduce Motion behave as the docs describe.
[Automated review]
Batch: L-check · Lab: CircleCheckIndicator
Size: 1,645 added lines, one Lab component, 879 of them tests.
User need
iOS marks multiple selection with a circle (Mail and Files edit modes,
.multiselect()lists). Astryx has only the box; rounded, it has neither UIKit's clear ring nor its checkmark.Evidence
Recorded on #8255's code; Testing's engine runs prove this head. UIKit's circle beside this one, iOS 26.2 at 3×:
A held finger, iOS 26.2: a square press before, a round one after. Android 15 Chrome, that press at ½ speed:
Every state, both sizes; then Chromium at this head, a mouse (round press, mark fading in) and a finger:
Problem and solution fit
CircleCheckIndicatora theme maps tocheckbox: an empty ring, or a filled circle with a checkmark (a bar when partial). Every CheckboxInput and CheckboxList then draws it, with its semantics and focus ring.Current specification
Lab code needs no record. Timing follows the theme's motion tokens and
design:reduced-motion; the press uses the internal marker #9716 added.Public delta and caller burden
A new Lab component,
CircleCheckIndicator(Indicator props), with a story, a CLI discover entry and a registry route; a theme opts in withindicators: {checkbox: CircleCheckIndicator}.Design and alternatives
Supersedes #8255.
Not in this PR: #8212 (the CheckboxList row press, which hands the row's paint to the circle and lands first; this then restacks and its held-label case flips) and #8701 (mouse and pen, after #8682).
Scope
Testing
Local gates at df21f7c, on main a814877:
pnpm build(no generated drift),verify-exports,pnpm typecheck(with the Lab and Storybook typecheck) andpnpm review:pre: clean.pnpm linton the changed files and CI's repo checks:sync-exports --check, copyright, the CLI API types, token docs, the production registry gate,check-knowledge,lab:readiness:check,check:motion-roles,check:changesetsand changeset coverage: clean.