Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsSkeleton (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | 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.
Approval needs an engineering owner to provide or identify the governing spec for this addition: no current spec decides how Skeleton presents under a coarse pointer, and the description itself asks for owner review of the still rest at 0.925. What must be settled is the rule's reach (it also applies to Android phones and tablets and to coarse-pointer screens 1024px and wider), its weight (0.925 under Reduce Motion too, about 2.07:1 in neutral dark) and the placeholder showing at full weight from its first frame; an engineering owner records the decision by commenting /approve-spec followed by the full head commit SHA on this pull request, which the author may do as an engineering owner. Separately, pr-a11y fails on a pinch-zoom contract step that main added after this branch was cut, which also skipped the new Skeleton contract, so rebasing onto main should clear it.
[Automated review]
| const classes = Array.from(screen.getByTestId('sk').classList); | ||
| const stops = classes.some(cls => | ||
| new RegExp( | ||
| `@media[^{]*\\(pointer: coarse\\)[^{]*\\{\\s*\\.${cls}\\b[^{]*\\{[^}]*animation-name:\\s*none`, |
There was a problem hiding this comment.
This pattern also matches the reduced-motion rule, which StyleX compiles to @media (prefers-reduced-motion: reduce) and (not (pointer: coarse)), so the test stays green if the coarse animationName key goes back to skeletonFade. Anchoring it the way the opacity check below is anchored (@media \(pointer: coarse\)) makes it fail in that case.
| } | ||
| }); | ||
|
|
||
| test('a resting placeholder lays out and paints nothing', async ({ |
There was a problem hiding this comment.
On the base, where the pulse still runs under a coarse pointer, this trace also counts 0 Paint and 0 Layout events on the renderer main thread, because the opacity pulse runs on the compositor, so the test passes with or without the change. The getAnimations() check in the first test is what detects the rest.
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for this addition: no current spec decides how Skeleton presents under a coarse pointer, and the description itself asks for owner review of the still rest at 0.925. What must be settled is whether Skeleton rests still under (pointer: coarse), its 0.925 weight (which also replaces the 0.25 rest under Reduce Motion and during the first second) and its reach to Android and to coarse-pointer screens 1024px and wider; an engineering owner records the decision by commenting /approve-spec followed by the full head commit SHA on this pull request, which the author may do as an engineering owner. Separately, pr-a11y stops at a pinch-zoom step that main added after this branch was cut, so the new Skeleton contract has not run in CI; rebasing onto main should clear it. The two earlier inline notes on the unit-test pattern and the paint trace still apply, as optional tightening.
[Automated review]
| })); | ||
| expect(colors.scheme).toBe('dark'); | ||
| // Storybook's default theme: light-dark(#f1f1f1, #525252). | ||
| expect(colors.fill).toBe('rgb(82, 82, 82)'); |
There was a problem hiding this comment.
This pins the neutral palette's dark skeleton fill, so regenerating that palette would fail the Skeleton touch contract without any Skeleton change. The colorScheme assertion two lines up already shows dark mode applied, so this line can go.
…ers change at once (#8272) ## Exact decision needed What a person who prefers reduced motion sees when a core component animates a change, on touch and on other pointers, and whether a component may keep its motion. ## Existing authority searched - `design:motion` DR5 (draft): every animated transition has an immediate or minimally moving form. Draft, so not authority. - `spec:AST-013` FR13: reduced-motion behavior is unchanged by server-rendered entrances; it defines none. - `spec:AST-012` FR2: `motion: reduce` and `pointer: coarse/fine` are the adaptation conditions. - No current web record defines component behavior under reduced motion. - Search terms: reduced motion, prefers-reduced-motion, cross-fade, Reduce Motion, motion DR5, spring, travel. Overlapping open pull requests: #7367 (draft `design:motion` DEC-2, hover entrances; no reduced-motion rule), #8214 (layer fade under a finger with Reduce Motion; conforms once its fade is 200 ms or less), #8052 (AST-094 FR8 carries the matching touch rule), #8211, #8239, #8253 (touch indicator and placeholder work; continuous indicators are outside this record). ## Decision New current record `design:reduced-motion`, matching iOS Reduce Motion with Prefer Cross-Fade: - DR1: under `prefers-reduced-motion: reduce`, a core component's entrances, exits, presentation switches, and state changes never move, scale, resize, or spring. - DR2: on a coarse pointer, they cross-fade: opacity only, 200 ms or less. - DR3: on any other pointer, they complete at once. - DR4: no core component, prop, or theme value keeps motion under reduced motion. `spec:AST-057` FR5 names that a swiped row's slide out, spring back, and hold-out follow DR2 under reduced motion. The draft `design:motion` keeps its other rules unapproved; its DR5 points to the new record. A separate record keeps the promotion to this one rule, as `docs/design/README.md` advises for a different approval lifecycle. ## Rejected alternatives - A per-component setting that keeps full motion under reduced motion. ## Claim scope and non-goals Only transitions under reduced motion. Motion without the preference, durations and easing, tokens, continuous indicators such as a spinner, and content the person drags with their own finger are outside this record. ## Evidence and implementation relationship Layers that open under a finger with Reduce Motion appear in one frame today, where iOS cross-fades. The open fix #8214 makes them fade under a coarse pointer and stay immediate under a mouse, which conforms to DR2 and DR3 once its fade is 200 ms or less. The open draft #7367 (`design:motion` DEC-2, hover-summoned layers) does not touch reduced motion. The AST-094 revision in #8052 carries the matching touch rule. No implementation is included. ## Writing rubric results - R1 narration: none. - R2 research in the record: none. - R3 evidence naming a change: none; verification names browser states. - R4 rejected alternatives over one line or outside a decision: one, inside DEC-1, one line. - R5 pull request or issue references remaining: none. - R6 ruled open questions still listing options: none. ## Scope - [x] This PR contains specification records only. - [x] It records observable behavior and preserves equivalent internal implementations. - [x] Any named internal mechanism is intentionally public, names its dependent caller/system, and explains why an equivalent implementation would not satisfy the contract. - [x] Verification states the evidence layer and failure signal without prescribing CI job/workflow topology. - [x] It records one intentional durable decision, not a rescue for a separable tagalong. - [x] The canonical knowledge owner, affected current claims, and overlapping open work are named; any ownership collision is resolved or called out for human decision. - [x] Adjacent API, visual, accessibility, composition, and implementation facts remain outside scope unless this exact decision depends on them. - [x] Public text and artifacts contain no internal Meta context. ## Validation - `pnpm check:knowledge` (CI lint) - Prettier - `git diff --check`
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for this addition: no current spec decides how Skeleton presents under a coarse pointer, and the description itself asks for owner review of the still rest at 0.925. What must be settled is whether Skeleton rests still under (pointer: coarse), its 0.925 weight (which also replaces the 0.25 rest under Reduce Motion and during the first second, and reaches 2.07:1 in neutral dark) and its reach to Android and to coarse-pointer screens 1024px and wider; an engineering owner records the decision by commenting /approve-spec followed by the full head commit SHA on this pull request, which the author may do as an engineering owner. These need fixing whatever the decision:
pr-a11yfails on this head atSkeletonTouch.a11y.chromium.spec.ts:142, because the Default story has one placeholder and two samples 300 ms apart can land on the same step of its alternating pulse; take more samples or check that the animation'scurrentTimeadvances.- The description should report the
@astryxdesign/corepackage stylesheet's gzip size at base and head, which AST-080 FR8 requires beside the component figures. - The contract step's scope pattern (
ci.yml:2321) should includeapps/storybook/stories/Skeleton.stories.tsx, the story the contract loads.
[Automated review]
| expect(count).toBeGreaterThan(0); | ||
| expect(animations.every(n => n === 1)).toBe(true); | ||
| // The pulse moves the opacity between samples. | ||
| expect(first.some((value, i) => value !== second[i])).toBe(true); |
There was a problem hiding this comment.
This fails in pr-a11y on this head (Expected: true, Received: false) with the pulse running. The Default story has one placeholder, and the alternate 400 ms pulse reads the same step at two samples 300 ms apart whenever the first lands 240–260 ms or 640–660 ms into its 800 ms cycle. Taking three samples 150 ms apart, or asserting that getAnimations()[0].currentTime advances, removes the timing dependence.
| && git show "$BASE_SHA:.github/scripts/pr-scope-touches.sh" > "$helper"; then | ||
| scope=0 | ||
| bash "$helper" "$BASE_SHA" \ | ||
| '^packages/core/src/Skeleton/' || scope=$? |
There was a problem hiding this comment.
The contract opens core-skeleton--default from apps/storybook/stories/Skeleton.stories.tsx, which this pattern does not match, so a story-only change skips the contract. The sibling contract steps list their stories; '^(packages/core/src/Skeleton/|apps/storybook/stories/Skeleton\.stories)' would cover it.
|
The still rest stays on |
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for this addition: no current spec decides how Skeleton presents under a coarse pointer, and the description itself leaves the still rest at 0.925 pending owner review. What must be settled is whether Skeleton rests still under (pointer: coarse), its 0.925 weight (which also replaces the 0.25 rest under Reduce Motion and during the first second, and reaches 2.07:1 against the card in neutral dark) and its reach to Android and to coarse-pointer screens 1024px and wider; an engineering owner records the decision by commenting /approve-spec followed by the full head commit SHA on this pull request, which the author may do as an engineering owner. The held-time pulse check and the reported stylesheet size resolve two earlier items, but whatever the decision, the contract step's scope pattern (ci.yml:2321) should also match apps/storybook/stories/Skeleton.stories.tsx, the story the contract loads. If the ruling keeps 0.925 under Reduce Motion, the changeset should say so.
[Automated review]
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for this addition: no current spec decides how Skeleton presents under a coarse pointer, and the cited AST-094 is still a proposal that leaves each component's touch presentation to that component's own record. What must be settled is whether Skeleton rests still under (pointer: coarse), its 0.925 weight (which also replaces the 0.25 rest under Reduce Motion and during the first second, and reaches 2.07:1 against the card in neutral dark) and its reach to Android, tablets and coarse screens 1024px and wider; an engineering owner records the decision by commenting /approve-spec followed by the full head commit SHA on this pull request, which the author may do as an engineering owner. Whatever the decision, these need fixing, ideally before the approval since it names one exact head:
- The contract step's scope pattern (
ci.yml:2563) should also matchapps/storybook/stories/Skeleton.stories.tsx, the story the contract loads. - If the ruling keeps 0.925 under Reduce Motion, the changeset should say so, since AST-017 FR3 asks it to name the old and new appearance.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] On a touch screen, Skeleton rests still instead of pulsing, as iOS placeholders (SwiftUI's redacted views) do, at the pulse's top step: about the weight of SwiftUI's placeholder, where the 0.25 rest is barely visible on a phone. With a mouse or trackpad the pulse is unchanged, and reduced motion still stops it everywhere. |
There was a problem hiding this comment.
Under Reduce Motion on a touch screen the rest also moves from 0.25 to 0.925 (1.18:1 to 2.07:1 against the card in neutral dark), which "reduced motion still stops it everywhere" does not tell readers. If the ruling keeps that, naming it here meets AST-017 FR3, which asks the changeset to name the old and new appearance.
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval needs an engineering owner to update the existing Skeleton spec for this change: component:Skeleton is now current and records a 0.25 rest and a pulse on every pointer (FR3, FR4, AR2 and its Reduced motion state), so the still 0.925 rest under (pointer: coarse) needs those claims amended, settling its reach to every coarse pointer, its weight, Reduce Motion and the first frame, or ruled out. An engineering owner records the decision by commenting /approve-spec followed by the full head commit SHA on this pull request, which the author may do as an engineering owner. Whatever the ruling, two things need fixing first, since the approval names one exact head:
- The latest merge brought the touch-foundation chain from #8076's branch into this pull request (about 30,000 lines, including a public
defineThememotion API), and the branch now conflicts withmainin nine gesture and motion files; please rebase so it carries only the five Skeleton files. - If the ruling keeps 0.925 under Reduce Motion, the changeset should say so rather than "reduced motion still stops it everywhere".
[Automated review]
…fter layout rounding Lane 0's real-host browser spec found a strip authored at the 40.2 px edge region lays out at 40.203 px (layout snaps to 1/64 px), so the strict comparison read it as off the zone and the pan decided at 10 px instead of UIKit's 15 (spec:AST-094 FR18). The zone takes half a pixel of tolerance: the rounded strip is the zone, a view a pixel wider still is not. Failing-first test at 40.203 px: it claims pan-x and decides at 15. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
There was a problem hiding this comment.
Approval is waiting on an engineering owner's decision about component:Skeleton: its current FR3, FR4 and AR2 and its Default, Reduced motion and Forced colors states record a 0.25 rest and a pulse on every pointer, and this pull request intentionally replaces them with a still 0.925 rest under a coarse pointer. An engineering owner can accept that by amending those claims, for example in this pull request, and commenting /approve-spec <full head SHA> (the author may do so as an engineering owner), or can rule the touch rest out; the decision should settle the reach (every coarse pointer, Android and wide coarse screens included), the weight across themes, the rest under Reduce Motion and the rest from the first frame. Independently of that decision, the branch carries #8076's 16,047 lines beside the 239-line Skeleton change, so please rebase onto main once #8076 lands.
[Automated review]
Main's meta#8521 is its head 25b468c squashed, which this pull request already carries: every file it touches is byte-identical to 25b468c. In the two files that conflicted (the motion README and animator.ts), the resolution is this side's version, main's merged meta#8521 with this pull request's later work on top. Nothing of main is dropped.
An attach reads the element's own will-change in a task of its own. A shared animator (animatorFor) is never detached, so its node can leave the document with that read still waiting, and the read then forces a style pass on a node with no style: a test run that mounts and unmounts many pagers piled up such reads until a later wait timed out. The read now skips a node out of the document; a promotion that needs it reads it then, as one on a node attached in the same task does. A detach and dispose already cancel it. The new test fails on the previous code (one read of a removed node); the in-task read test now puts its element in the document, where the read happens.
There was a problem hiding this comment.
Approval is waiting on an engineering owner's decision about component:Skeleton: its current FR3, FR4 and AR2, with its Default, Reduced motion and Forced colors states, record a 0.25 rest and a pulse on every pointer, and this pull request intentionally replaces them with a still 0.925 rest under a coarse pointer. An engineering owner can accept that by amending those claims, for example in this pull request, and commenting /approve-spec <full head SHA> (the author may do so as an engineering owner), or can rule the touch rest out; the decision should settle the reach (every coarse pointer, Android and wide coarse screens included), the weight across themes, the rest under Reduce Motion and the rest from the first frame. Independently of that decision, the branch carries open #8076 (15,622 of its 15,861 changed lines) beside the 239-line Skeleton change, so please rebase onto main once #8076 lands.
[Automated review]
…(spec:AST-094 FR18, DEC-13) A screen-edge pan attached to a stack container, as wide as the screen, with a touch 4 px from its edge: inside the 40.2 px band the pan decides at UIKit's 15 px whatever the element's width, and then tracks the travel past it. From several edges too. Both fail while a wider element decides at FR19's 10 px.
…'s width (spec:AST-094 FR18, DEC-13) FR18: inside the 40.2 px band a screen-edge pan decides at UIKit's 15 px screen-edge hysteresis; outside it the author's scrolling is untouched and FR19's 10 px governs every other claim. The pan decided at 10 px on any element wider than the band, so NavigationStack's edge pan, attached to the stack container, began 5 px early. The edge zone still decides only the touch-action; a touch outside every band still fails at once. The two cases that pinned the 10 px decision go.
…er's move keeps that finger until it lifts) into the experimental touch exports
…than the band (spec:AST-094 FR18, DEC-22) FR18 as amended in meta#8631 (DEC-22): the band's pan-x claim and 15 px start hold only where the screen-edge pan is attached to an element within the band; on an element that reaches beyond it, whose touch-action cannot claim per touch, a touch in the band decides at FR19's 10 px like every other claim. This restores the 10 px decision 3d85c30 removed and keeps 15 px on an element within the band. The cases pin DEC-22 both ways (a wide element with a touch at x=4 begins at 10, an in-band strip at 15, and from several edges at 10 on a wide element); the 10 px cases fail on 3d85c30.
…xperimental touch exports The motion README's Files table is the union: main's rows, with meta#8663's resolveSnapPoint.ts and selectPage.ts, plus this branch's, in main's order. # Conflicts: # packages/core/src/motion/README.md
The gesture README promises that usePressTracker rebuilds its tracker when trackingExpansion, highlightDelay, cancelTravel or quickTapFlash change, from the next touch. That fix (c248459) was lost in a later merge of the press-tracking line: the tracker was built once, so a changed option never applied. It is memoized on those four again; the node attachment, keyed on the tracker, moves to the new one and disposes the old. The callbacks keep being read through the ref on every touch. The new test fails before this change (the same tracker after each option change) and passes after it.
There was a problem hiding this comment.
Approval is waiting on an engineering owner's decision about component:Skeleton: its current FR3, FR4 and AR2, with its Default, Reduced motion and Forced colors states, record a 0.25 rest and a pulse on every pointer, and this pull request intentionally replaces them with a still 0.925 rest under a coarse pointer. An engineering owner can accept that by amending those claims, for example in this pull request, and commenting /approve-spec <full head SHA> (the author may do so as an engineering owner), or can rule the touch rest out; the decision should settle the reach (every coarse pointer, Android and wide coarse screens included), the weight across themes, the rest under Reduce Motion and the rest from the first frame. Independently of that decision, the branch carries an earlier revision of open #8076 (15,794 of its 16,033 changed lines) beside the 239-line Skeleton change, so please rebase onto main once #8076 lands.
[Automated review]
… s of wall clock
The two proofs that a Reduce Motion dip held before its first frame
still lands (after the hold is finished, and after it is resumed) polled
the box for 2 s of wall clock. On a loaded machine the page's main
thread ran the dip's next step late and the poll ran out first: one
WebKit run read {opacity: 0, translate: 'none'} with four WebKit
containers on the box, then passed in 530 ms on a rerun. They now wait
until the box runs no animation, on the animations' own finished
promises (including the fade back in the dip starts when its fade out
ends), and then assert the landed state. The test's own timeout still
bounds them.
With a 2.5 s main-thread block induced right after the release, both
proofs failed before this change (Timeout 2000ms exceeded while waiting
on the predicate) and pass after, 3 runs each in Chromium.
Main merged meta#8045 and meta#8046. The one conflict is utils/pressFeedback.ts: it takes main's merged meta#8046 text (the finger and button slots, the mouse and key press) and keeps this pull request's own additions, the one writer of the press attribute (writePress, paintPress, readPressPaint) and the header line naming them.
# Conflicts: # internal/interaction-harness/README.md # internal/interaction-harness/src/busy.test.ts # internal/interaction-harness/src/busy.ts # internal/interaction-harness/src/frames.ts # internal/interaction-harness/src/harness.interaction.browser.spec.ts
Main's button slot, merged with meta#8046, ends a press's release at once when the next press starts, by clearing the press attribute directly. On a row another painter holds, that clear erased the hold, which this pull request's one writer refuses. A mouse press on a held row, released, then a press elsewhere must leave the row held. Fails until the early end goes through the one writer.
finish() writes none through writePress, as this pull request's controller paints, so a held row stays its owner's when the next press ends a release early, under the finger and the button slots alike.
Both upstream heads are folded onto main 70ea859 (her meta#8045 frame evidence and meta#8046 coarse press): meta#8446 takes main's harness files, and meta#8073 carries the press controller onto meta#8046's slots. meta#8076 has no change of its own in either set of files, so the merge takes theirs with no hand resolution, and meta#8076 is clean against main again.
There was a problem hiding this comment.
Approval is waiting on an engineering owner's decision about component:Skeleton: its current FR3, FR4 and AR2, with its Default, Reduced motion and Forced colors states, record a 0.25 rest and a pulse on every pointer, and this pull request intentionally replaces them with a still 0.925 rest under a coarse pointer. An engineering owner can accept that by amending those claims, for example in this pull request, and commenting /approve-spec <full head SHA> (the author may do so as an engineering owner), or can rule the touch rest out; the decision should settle the reach (every coarse pointer, Android and wide coarse screens included), the weight across themes, the rest under Reduce Motion and from the first frame, and the forced-colors pulse under a finger, which the description and one contract test still call unchanged. Independently of that decision, the branch carries open #8076 (14,048 of its 14,287 authored changed lines) beside the 239-line Skeleton change, so please rebase onto main once #8076 lands.
[Automated review]
| } | ||
| }); | ||
|
|
||
| test('forced colors are unchanged: GrayText at full opacity, still', async ({ |
There was a problem hiding this comment.
On the base, under a finger with motion allowed, forced colors pulses from 0.25 to 0.925 (component:Skeleton AR2); this test asserts the new still rest at 1. The title, and the description's state matrix, could say that forced colors stops pulsing under a finger rather than that it is unchanged.
Review only this PR's change: vjeux/astryx@6f994bc...50c63a0
User impact
Part of making Astryx match iOS under a finger: on a phone, a Skeleton pulses between 0.25 and 0.925 opacity, a desktop pattern, while iOS placeholders stay still (SwiftUI's placeholder, in light, dark and Reduce Motion). Now, under a finger (
spec:AST-094FR13), Skeleton rests still; a mouse or trackpad keeps the pulse.Options and tradeoffs for the rest:
Themes keep their colors (Storybook's lighter default rests at 1.12:1, its pulse's peak).
Visual authority
DDrow reused or amended.Design decisionstable by a team design owner.docs/design/.None is settled:
component:Skeleton(current) pulses on every pointer. Proposed amendment, pending owner review: under a finger, static at the top step, anchored on SwiftUI's placeholder.Before and after
Mobile Safari, iOS 26.2 simulator: main pulsing (top), this PR still (bottom):
Dark mode at rest, and Chrome on Android, still:
SwiftUI's placeholder (top) and this PR's rest (bottom):
State matrix
Size (gzip, Skeleton's import, base → head): JavaScript 3,861 → 3,874 B (+13), CSS 3,706 → 3,735 B (+29);
astryx.css35,381 → 35,398 B (+17).Scope
Testing
What I ran and what it showed: