Repository navigation
feat(core): a first-party indicator can paint its own press - #9716
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| MultiSelector | 73.1 kB (+47 B) · 15.3 kB CSS (+67 B) | — | — · — · — |
| TableHeaderCell | 75.9 kB (+45 B) · 15.6 kB CSS (+69 B) | — | — · — · — |
| Table | 84.5 kB (+41 B) · 16.1 kB CSS (+71 B) | — | — · — · — |
13 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Markdown | 145.2 kB (+45 B) · 18.3 kB CSS (+65 B) | — | — · — · — |
| TableCell | 76.4 kB (+42 B) · 15.8 kB CSS (+64 B) | — | — · — · — |
| CheckboxListItem | 53.5 kB (+34 B) · 11.1 kB CSS (+70 B) | — | — · — · — |
| ContextMenu | 75.2 kB (+33 B) · 15.4 kB CSS (+69 B) | — | — · — · — |
| ListItem | 52.8 kB (+30 B) · 11.1 kB CSS (+70 B) | — | — · — · — |
| CheckboxInput · Storybook | 48.8 kB (+32 B) · 9.7 kB CSS (+64 B) | 374 (+13) | — · — · — |
| MoreMenu | 73.4 kB (0) · 15.5 kB CSS (+62 B) | — | — · — · — |
| DropdownMenu | 73.1 kB (−1 B) · 15.5 kB CSS (+62 B) | — | — · — · — |
| lab/Comments | 232.9 kB (+49 B) · 2.5 kB CSS (0) | — | — · — · — |
| lab/MobilePowerSearch | 214.6 kB (+46 B) · 1.2 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditor | 277.8 kB (+43 B) · 2.6 kB CSS (0) | — | — · — · — |
| richtext/RichTextView | 258.3 kB (+41 B) · 2.3 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 227.6 kB (+34 B) · 2.4 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.11 MB packed (+1.4 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: core 50 rows; run pnpm api:report.
Wide change (shared styles: packages/core/src/internal/indicatorPress.stylex.ts, packages/core/src/theme/derivedVarRegistry.test.ts): add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change does what it says: the two new cases fail without it, and a round replacement indicator that applies indicatorPressScope loses CheckboxInput's square press layer and paints the handed-down touch press itself.
Approval waits on an engineering-owner decision, because this pull request intentionally changes an existing spec:
component:CheckboxInputAV1 and FR11 say CheckboxInput paints pressed feedback on a replaced indicator; an engineering owner needs to approve amending them so an indicator withindicatorPressScopepaints its own press while CheckboxInput keeps painting focus.- The new
indicatorPressScopeexport in@astryxdesign/core/Indicator/indicator.markers.stylexhas no governing spec; the same decision should settle whether it is public for every theme or for Astryx's own indicators only.
An engineering owner records the decision by commenting /approve-spec followed by the full head SHA on this pull request; as an engineering owner, the author may do so.
Required change:
- The changeset offers
indicatorPressScopeto every theme indicator, while its JSDoc limits it to Astryx's own indicators and the touch press reaches it only through the private--_indicator-press-image, so make the changeset, the export's JSDoc or tag, and the variable's privacy agree with the owner's decision.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] A theme indicator can paint its own press layer: CheckboxInput leaves its rounded-square press layer off over an indicator marked with the new `indicatorPressScope` marker (`@astryxdesign/core/Indicator/indicator.markers.stylex`), so a round indicator's press stays round. Indicators without the marker are unchanged. |
There was a problem hiding this comment.
This offers indicatorPressScope to every theme indicator, but the marker's JSDoc limits it to Astryx's own indicators, and the touch press reaches a marked indicator only through the private --_indicator-press-image. Please word this for the audience the owner decides, or tag the export @internal if it stays first-party.
| // Off when the indicator paints its own press: this rounded square | ||
| // would tint past a round shape. |
There was a problem hiding this comment.
Non-blocking: this repeats the comment above. The quirk worth a comment is the ':is(*)' argument, for example: "when.descendant takes a pseudo-class. :is(*) matches any marked indicator."
bfa418e to
fd5f423
Compare
vjeux
changed the title
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec fd5f423 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Requesting one change: the component:CheckboxInput FR11 and AV1 amendment is owner-approved, and the new tests fail without the change and pass with it, but the release note overstates what ships.
.changeset/indicator-own-press.mdtells@astryxdesign/coreconsumers that Lab'sCircleCheckIndicatorgets a round press, but no indicator in this release appliesindicatorPressScopeand Lab is not published, so describe only what Core ships here, for example "CheckboxInput can leave its rounded-square press layer off over an Astryx replacement indicator that paints its own press; indicators in this release are unchanged."
Non-blocking: the last sentence of the comment above indicatorPressImage in CheckboxInput.tsx packs two topics; a suggested rewrite is inline.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] CheckboxInput leaves its rounded-square press layer off over a first-party indicator that paints its own press, such as Lab's CircleCheckIndicator, so a round indicator's press stays round. Other indicators are unchanged. |
There was a problem hiding this comment.
This names Lab's CircleCheckIndicator and a round press, but no indicator in this release applies indicatorPressScope and Lab is not published, so core consumers read about a change they cannot get. Suggested:
[feat] CheckboxInput can leave its rounded-square press layer off over an Astryx replacement indicator that paints its own press. Indicators in this release are unchanged.
| // 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: this sentence packs two topics and does not name the row it means. Suggested: "Only this row declares it. A menu item paints its own press, so a mapped indicator there paints none."
fd5f423 to
f730ea9
Compare
|
/approve-spec f730ea9 |
There was a problem hiding this comment.
Requesting one change: the component:CheckboxInput FR11 and AV1 amendment is owner-approved, and the new tests fail without the change and pass with it, but the release note still describes something this release does not ship.
.changeset/indicator-own-press.mdtells@astryxdesign/coreconsumers that Lab'sCircleCheckIndicatorgets a round press, but no component of that name exists, no indicator in this release appliesindicatorPressScope, and Lab is not published, so describe only what Core ships, for example: "CheckboxInput can leave its rounded-square press layer off over an Astryx replacement indicator that paints its own press. Indicators in this release are unchanged."
Non-blocking: the last sentence of the comment above indicatorPressImage in CheckboxInput.tsx packs two topics; for example, "Only this row declares it. A menu item paints its own press, so a mapped indicator there paints none."
[Automated review]
f730ea9 to
c1a0001
Compare
|
/approve-spec c1a0001 |
There was a problem hiding this comment.
Recommending approval: the component:CheckboxInput FR11 and AV1 amendment is owner-approved for this revision, the new tests fail without the change and pass with it, and the release note now describes only what Core ships.
No blocking findings remain, and the earlier changeset and comment suggestions are addressed.
[Automated review]
c1a0001 to
760550c
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 760550c |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Recommending approval: the component:CheckboxInput FR11 and AV1 amendment is owner-approved for this revision, the new tests fail without the change and pass with it, and built-in and unmarked indicators keep today's press.
No blocking findings.
[Automated review]
cixzhang
added
the
ci:run
CheckboxInput paints its press over a replacement indicator as a rounded square, which tints past a round indicator such as a circle. It now leaves that layer off over an indicator that applies `indicatorPressScope`, an internal marker for Astryx's own replacement indicators (`@astryxdesign/core/internal/indicatorPress.stylex`, not public API). The row also hands its touch press image down as `--_indicator-press-image`, because an indicator in another package may not name the row's private press attribute. Only the row declares it, so an indicator mapped into a row that paints its own press, such as a menu's checkbox item, paints nothing twice. Hover and a mouse press reach the indicator through the row's scope marker, as for the built-in indicator. Indicators without the marker are unchanged. component:CheckboxInput AV1 and FR11 now say so: a first-party replacement indicator may paint its own press from the row's hover and press, while CheckboxInput still paints focus. The unit cases that read the injected rules fail on main: the row hands its touch press image down, and the press layer turns off over a marked indicator. A disabled row hands nothing down. A unit case keeps the marker off every public entry and the API report.
760550c to
b5ae87c
Compare
There was a problem hiding this comment.
Recommending approval: the component:CheckboxInput FR11 and AV1 amendment is owner-approved for this spec content, and CheckboxInput now leaves its rounded-square press layer off only over a first-party indicator that carries the internal marker, while built-in and theme indicators keep the owner-painted press.
- The new hand-down and layer-off cases fail without the change and pass with it, and in Chromium under a coarse pointer the pressed built-in and unmarked rows render identically before and after.
- No blocking findings; one optional comment-wording suggestion is 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: "reaches it no more than it reaches the built-in box" is a vague comparison. The reason it carries is that the arms read only this row's own attribute. Suggested: "The arms read only this row's attribute, so a pressed card around the row hands nothing down."
|
/approve-spec b5ae87c |
User need
A theme can replace CheckboxInput's indicator, but CheckboxInput paints the press over it as a rounded square, so a round indicator such as Lab's selection circle shows a square press.
Evidence
Unit, red first, in
CheckboxInput.test.tsx(reading the rules StyleX injects):--_indicator-press-image, only in its own press states;The first two fail on main, the first also on c1a0001 (a pressed ancestor row reached the indicator), and the third where the marker was public.
The stacked Lab batch proves it in Chromium and WebKit.
Problem and solution fit
indicatorPressScope, an internal marker (@astryxdesign/core/internal/indicatorPress.stylex), marks a first-party indicator that paints its own press.Current specification
As the spec owners directed,
component:CheckboxInputAV1 states the one exception: "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." FR11, the Behavior row and the ownership bullet cite AV1.Writing rubric results
Public delta and caller burden
--_indicator-press-image, documented as private.Design and alternatives
The Lab batch on top supersedes #8255.
Open drafts #8219, #8233 and #8696 gate this layer on
paintsPress, and #8212 makes the CheckboxList row its box's touch press surface. Whichever lands second gatesindicatorPressImagetoo, or hands down the row's paint (var(--_checkbox-row-press-paint, none)).Scope
Testing
Local gates at b5ae87c, on main bf14580:
pnpm build(no generated drift),node scripts/verify-exports.mjs,pnpm typecheckandpnpm review:pre: clean.node scripts/sync-exports.js --check(it fails on the previous head, 760550c, whose exports list lacked the internal entry),add-copyright.sh --check,cli-api-types-verify,generate-token-docs --check, the production registry gate andcheck-knowledge: clean.pnpm linton the changed files, Prettier,check:changesets,check:repo,check:i18n-runtimeand changeset coverage: clean.CheckboxInput.test.tsx,customPropertyArmLayering.test.ts,pressableCoverage.test.ts,derivedVarRegistry.test.tsandpublicStyleXNamespaces.test.ts; red first as listed under Evidence.pnpm api:report --checkflags main's own Tokenizer rows as stale; this change adds no row.