Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNew ComponentsRefreshControl (@astryxdesign/lab) · 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.
This pull request is on hold for an owner decision: its indicator values are marked proposed and pending owner review and rest on the draft spec:AST-094, which also amends the current theme-tokens record, user-states DR1 and AST-025 FR14 (its FR4, FR11 and FR14). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request once the draft is approved as the governing spec for these values and those existing records are updated for its amendments (or the amendments are dropped), or by saying which claims or values must change; the author may do so if they are an engineering owner. Independently of that decision, please:
- Split the branch: it carries 30,264 authored lines from the pull requests beneath it (#8045, #8074, #8062, #8098, #8175), so land those first, rebase, and leave only the 1,029-line drawing commit here.
- Derive a controlled
isRefreshingduring render inuseRefreshControl.ts(lines 677–684) instead of copying it into state from an Effect, soaria-busyis right on the first render, and name what the state-setting Effects synchronize. - Render the
titleoutside thearia-hiddenindicator inRefreshControl.tsxso screen readers can reach it.
[Automated review]
| useEffect(() => { | ||
| if (isRefreshingProp === undefined) { | ||
| return; | ||
| } | ||
| dispatch( | ||
| isRefreshingProp ? {type: 'begin', source: 'program'} : {type: 'end'}, | ||
| ); | ||
| }, [dispatch, isRefreshingProp]); |
There was a problem hiding this comment.
A controlled isRefreshing only reaches state through this Effect (dispatch runs the begin effect, which calls setIsRefreshing(true) at line 527), so with isRefreshing={true} at mount the first render returns isRefreshing: false and RefreshControl paints no aria-busy, then repairs it on the next render. The repository's React runtime architecture (INV1) asks that a component never render a known-wrong state and repair it in an Effect, and INV3 asks each Effect to name what it synchronizes. Deriving the value during render when controlled keeps it right from the first render:
const isRefreshingNow = isRefreshingProp ?? isRefreshing;The Effect can then stay for the inset, the spin attribute and the announcement, with a comment naming them.
| useEffect(() => { | ||
| setIsLocked(isScrollLockActive()); | ||
| return subscribeScrollLock(setIsLocked); | ||
| }, []); |
There was a problem hiding this comment.
This Effect copies the scroll-lock store into state without naming it (INV3 of the React runtime architecture). useSyncExternalStore(subscribeScrollLock, isScrollLockActive, () => false) would read the store during render and drop the Effect.
| {title != null ? ( | ||
| <span {...stylex.props(styles.title)}>{title}</span> | ||
| ) : null} |
There was a problem hiding this comment.
title renders inside the indicator, which is aria-hidden (line 548), so text such as “Updated just now” is visible during a pull or a refresh but never reaches a screen reader. Keeping the glyph hidden and rendering the title outside it, or folding it into the “Refreshing” announcement, keeps that information available to everyone.
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision: its RefreshControl indicator values are marked proposed and pending owner review, and they rest on draft spec:AST-094, which also amends the current theme-tokens, user-states (DR1, DR3), buttons, interaction-modality and AST-025 FR14 records. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request once the draft is approved as the governing spec for these values and those records are updated for its amendments (or the amendments are dropped), or by saying which claims or values must change; as an engineering owner, the author may do so. Independently of that decision, please:
- Split the branch: it carries 30,366 authored lines from #8045, #8074, #8062, #8098 and #8175, and its copies of #8045 and #8074 are already older than those pull requests, so land them first and leave only the drawing here.
- Put the
@astryxdesign/core/motion/experimentaland@astryxdesign/core/gestures/experimentalexports behind a boundary AST-017 FR21 recognises (an owning component's/experimentalsubpath), or have FR21 amended to admit module-level subpaths. - Derive a controlled
isRefreshingduring render inuseRefreshControl.ts(lines 677–684) soaria-busyis right on the first render, and name what the state-setting Effects synchronize. - Render the
titleoutside thearia-hiddenindicator inRefreshControl.tsxso screen readers can reach it. - Run the interaction proofs under an existing CI lane instead of the new self-routed
pr-interactionjob, as AST-030 FR11 requires.
[Automated review]
| @@ -72,6 +72,19 @@ const STATIC_EXPORTS = { | |||
| types: './dist/naming.d.ts', | |||
| default: './dist/naming.js', | |||
| }, | |||
| // Experimental subpaths (spec:AST-017 FR21): outside the stable | |||
There was a problem hiding this comment.
These two subpaths are not a boundary spec:AST-017 FR21 recognises: it says experimental hooks, functions and types “use the owning component's /experimental import subpath”, and neither motion nor gestures is a component. Until an owner amends FR21 to admit module-level experimental subpaths, the next stable release would publish their 30 runtime exports and 53 types with no recognised experimental status, so FR25's compatibility default would apply despite the changesets' promise that they may change or be removed in a patch.
|
|
||
| # The helper comes from the base, since an older PR head lacks it. | ||
| # Exit 1 means out of scope; 0 in scope; 2 unknown, so run. | ||
| - name: Check interaction scope |
There was a problem hiding this comment.
This step gives pr-interaction its own routing for coverage of components in existing packages. spec:AST-030 FR11: “New coverage inside an existing surface MUST join those owners and MUST NOT create a separately routed lane or required context.” Running the interaction proofs under the owning surfaces' existing lanes (or getting the lane admitted in AST-030) would keep CI routing to one owner per surface.
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision: its RefreshControl indicator values are marked proposed and pending owner review, and they rest on draft spec:AST-094, which also amends the current theme-tokens, user-states (DR1, DR3), buttons, interaction-modality and AST-025 FR14 records. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, or by saying which claims or values must change; as an engineering owner, the author may do so. Independently of that decision, please:
- Split the branch: it carries 30,505 authored lines from #8045, #8074, #8062, #8098 and #8175, and its copies of #8045, #8074 and #8062 are already older than those pull requests.
- Put the
@astryxdesign/core/motion/experimentaland@astryxdesign/core/gestures/experimentalexports behind a boundary AST-017 FR21 recognises, or have FR21 amended to admit them. - Render the
titleoutside thearia-hiddenindicator, and derive a controlledisRefreshingduring render soaria-busyis right on the first render. - Let the scroll-lock touch guard pass one-finger pans while the page is pinch-zoomed, so people who zoom in can still reach every part of an open dialog.
- Remove the claim that a theme retunes
triggerGradientSpring, and drop--motion-refresh-control-spin-msunless the draft's FR4 is amended to allow it. - Run the interaction proofs under an existing CI lane (AST-030 FR11), and point AST-094 FR7 at a record that exists instead of AST-090.
[Automated review]
| spinTiming: { | ||
| willChange: 'opacity', | ||
| animationDuration: | ||
| 'calc(8 * var(--motion-refresh-control-spin-ms, 1000ms))', |
There was a problem hiding this comment.
Nothing in the repository declares, documents or writes --motion-refresh-control-spin-ms; it appears only here and in RefreshControl.test.tsx. A theme that did set it would retune a touch motion, which this pull request's own draft spec:AST-094 FR4 forbids (“A touch presentation MUST read its motion from the table on every theme”). The docblock's “a theme that retunes refreshControl.triggerGradientSpring retunes it” (lines 24–27) is also not the case: the hook reads the spring from the constant table (useRefreshControl.ts:85). Unless FR4 is amended, consider going back to the table's period (a literal, or a private --_ variable written from refreshControl.spinMs) and correcting the docblock.
There was a problem hiding this comment.
Both points depend on a theming path this branch already has, so I've left the docblock and the variable unchanged:
- The spring. The hook passes the table's spring object to the animator, and the animator resolves a table spring against the theme each time a segment starts: animator.ts L1301–L1302,
const resolved = resolveMotion(target, spring). So a theme that setsrefreshControl.triggerGradientSpringdoes change this cross-fade. Passing the table constant is how a role is read. - The variable.
refreshControlis registered as a motion table (refresh.ts L225), which makesrefreshControl.spinMsa role. The generated motion docs list it with--motion-refresh-control-spin-ms(motion.doc.mjs L200–L201), and a theme that sets the role writes that variable.
Touch motion is retunable per theme (spec:AST-094 FR4 and DEC-3, #8378), so both stay as they are.
| : 'cancel'; | ||
| } | ||
| if (verdict === 'cancel' && event.cancelable) { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
This cancels every one-finger pan that no scroller absorbs, and the guard never reads visualViewport.scale, so it also cancels the pan that moves a pinch-zoomed view. With a dialog open, someone who has zoomed in can still zoom but cannot pan to the dialog's header or actions (WCAG 2.2 SC 1.4.4), which the old body pin did not block. Consider letting the pan through while visualViewport.scale > 1, with a test beside the two-finger one.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
No spec:AST-090 record exists at this head or on main, so this departure (and the proposed amendment in Current-state impact, line 235) has nothing for its owners to rule on. Cite the record that holds those exit rules, or drop the departure.
e79bbbb to
5efe2d0
Compare
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision: its RefreshControl indicator values are marked proposed and pending owner review and rest on draft spec:AST-094, whose FR4 and DEC-1 forbid retuning touch motion per theme, while this branch now adds theme motion roles that do exactly that. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, or by saying which claims or values must change, including whether FR4 or the motion roles stand; as an engineering owner, the author may do so. Independently of that decision, please:
- Split the branch: it carries 34,299 authored lines from #8045, #8074, #8062, #8098, #8086, #8175, #8257 and #8258 for a 1,589-line drawing.
- Leave the
platformadaptation condition and the--motion-*role tokens to #8257 and #8258 until AST-012 FR2 and theme-tokens INV1 admit them, and validate role ids instead of keying a stabledefineThemefield by experimental table paths. - Put the two experimental core subpaths behind a boundary AST-017 FR21 recognises, or have FR21 amended to admit them.
- Derive a controlled
isRefreshingduring render, render thetitleoutside thearia-hiddenindicator, and let the scroll-lock guard pass one-finger pans on a pinch-zoomed page. - Run the interaction proofs under an existing CI lane (AST-030 FR11), and point AST-094 FR7 at a record that exists instead of AST-090.
[Automated review]
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
This adds a fifth field to the adaptation conditions. spec:AST-012 FR2 lists width, pointer, contrast and motion and says “Future environment fields require explicit additions to this closed vocabulary”, so platform needs that amendment from the theme owners (with #8257) before it ships, or it should leave this branch.
| const registered = motionRole(id); | ||
| if (registered !== undefined && registered.kind !== kind) { |
There was a problem hiding this comment.
An id that no table has registered passes this check and writes a --motion-* variable nothing reads, so a mistyped role, or one an experimental table renames in a patch, silently stops applying to a stable theme. Validating ids against the registered tables (or keeping role keys out of the stable defineTheme surface until the tables are stable) keeps theme authors from shipping motion settings that do nothing.
|
|
||
| // Each spring, duration and curve above is a motion role a theme may retune | ||
| // (motion/roles.ts). | ||
| registerMotionTable('refreshControl', refreshControl, { |
There was a problem hiding this comment.
Registering refreshControl makes spinMs, triggerGradientSpring, triggerTurnSpring and endMs theme roles, while this pull request's own draft spec:AST-094 FR4 says “A touch presentation MUST read its motion from the table on every theme” and DEC-1 rejects tuning touch motion per theme. One of the two has to give; that is the owner decision this review waits on.
…ulator (page area only) and Chrome on Android (below its bar)
|
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.
|
5efe2d0 to
42b5675
Compare
github-actions
Bot
added
the
needs:design-review
42b5675 to
dfc87c7
Compare
…ware GL), with meta#8251's drawing: a scroll area, and the page with no reload
…the flat tree isFingerDownIn and the pointer-events finger match compare with containsAlongFlatTree, as the scroll watch already does: a finger on content inside an open shadow root is down in the scroller around that root, and its touches, retargeted to the shadow host, match its pointer finger, so a claimed pan cancels its moves there (spec:AST-094 FR19, FR20).
…achments asks, and only then The host let the last attachment decide whether an element is an action group (useGestureRecognizer always passes the option), and never recomputed it at a detach: a plain attachment after the group's turned the group off, and a group's attachment released left a recognizer that never asked taking presses under a fine pointer. Fails until the host counts the attachments that ask.
…ents asks The host counts the attachments that pass actionGroup: true and keeps the element a group while one remains, recomputed at each detach. An attachment that passes false, as useGestureRecognizer does by default, no longer turns a group off, and a group's last asking attachment taking its leave ends the group.
… that content The presented hit test takes the document's elementFromPoint, which answers a shadow host for anything inside its open root, and compares with Node.contains, which stops at the boundary: a recognizer on a control inside a moving sheet's shadow root gets no finger, and the tap's click goes to the host. Fails until the hit test descends into open roots and compares along the flat tree.
Where a moving element is drawn away from its layout box, the host hit-tests what is drawn under the finger with the document's elementFromPoint, which answers a shadow host for anything inside its open root. The hit test now descends through each open root's own elementFromPoint and compares with the movers along the flat tree, so a recognizer on a control inside a moving sheet's shadow root gets its finger and the tap clicks that control.
…nt lets go The host's touch-action rules sit in an adopted sheet or a style element per document or shadow root, and were never removed, so a page whose last recognizer detached kept them, against spec:AST-094 FR24 (nothing touch-related remains after the last instance unmounts). Fails until a root's rules go with its last marked element.
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
jsdom serves no cross-origin sheet, so the catch around the indicator tests' stylesheet reader could only hide a real error.
…eencast delivered A loaded machine's screencast skips frames the compositor still presents, so the proof no longer counts delivered frames against the screencast's cadence. As in the harness's busyWindowProof, every delivered frame must show the fade progressing, and a gap between two changed frames is measured between the compositor's own frames, recorded with startCompositorTrace. A fade driven from the main thread still fails it.
…he page presented Two proofs failed on a loaded machine with no defect in the control: - the busy-main-thread settle counted the screencast's delivered frames against its cadence. It now hands the compositor's own frame record to busyWindowProof, whose verdict alone judges the settle; a settle paused before the block fails it. - the catch read the settle's position before the touch, which lands later under load. A capture listener now records where the settle stood as the finger landed; a catch that restarts from zero travel fails it.
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision: whether RefreshControl's trigger cross-fade may keep the copy of the spokes it makes itself as an exception to architecture:component-style-authoring INV7, recorded as an owner decision until Core's shared transient-copy mechanism exists, or must drop it. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request and saying where the exception is recorded, or by saying the copy must go; the author may do so as an engineering owner. Independently of that decision, please:
- Land or drop #8175 and the stack beneath it and rebase onto main, so this pull request carries the 1,715-line drawing instead of 36,501 authored lines.
- Make
useRefreshControl'sspinnerRefmatch its docs for custom indicators: it steps every child as one of eight spokes, so a one-child glyph vanishes for 125 ms of each turn and never passes 0.56 opacity. - Reach the animator and the motion helpers through
@astryxdesign/core/internal/touch, asarchitecture:react-component-runtimeINV13 says, or amend INV13 first. - Back the iOS Safari and Android Chrome claims with real-device evidence or narrow them to the simulator and emulator, and run the busy-thread proofs through the 1 s block that
spec:AST-094's FR5 verification names.
[Automated review]
| return; | ||
| } | ||
| const turnMs = refreshControl.spinMs; | ||
| spin.current = Array.from(glyph.children) |
There was a problem hiding this comment.
Every child of the spinnerRef element gets spoke index's keyframes here, and line 473 clones every child at the trigger, so a custom glyph, which RefreshControl.doc.mjs:67 offers the hook for, is stepped as UIKit spokes: a one-child glyph runs 0, 0.56, 0.48 … 0.08 opacity, invisible for 125 ms of every turn. Step only the drawing's own spokes (a data attribute on the eight spans), or narrow the docs and the spinnerRef comment to an eight-spoke glyph.
| spokeCopy.style.opacity = '1'; | ||
| copy.append(spokeCopy); | ||
| } | ||
| glyph.append(copy); |
There was a problem hiding this comment.
This appends a component-made copy of the spokes (a span and eight clones) for the 5.3 s fade, so the glyph holds 17 elements instead of 8. architecture:component-style-authoring INV7 allows a second copy only from one shared Core mechanism, and its gap clause covers only a layer's exit, presentation-switch and handoff copies and a reorderable collection's drag preview, so keeping it needs an engineering owner's recorded exception.
| import {refreshControl} from '@astryxdesign/core/internal/motion'; | ||
| import {scroll} from '@astryxdesign/core/internal/motion'; | ||
| import {registerTimingTable, timing} from '@astryxdesign/core/internal/motion'; | ||
| import {useAnimator} from '@astryxdesign/core/internal/motion'; |
There was a problem hiding this comment.
architecture:react-component-runtime INV13 routes the animator, the spring and curve helpers and UIKit's tables through @astryxdesign/core/internal/touch; importing them from internal/motion gives Lab a second internal route into the same modules. Re-export them from internal/touch and import them there, or amend INV13 first.
| }); | ||
| const cast = await startScreencast(page); | ||
| await page.waitForTimeout(150); | ||
| const block = await blockMainThread(page, 700); |
There was a problem hiding this comment.
spec:AST-094's FR5 verification asks for an animation that keeps advancing through a 1 s main-thread block; this proof, and the cross-fade's at line 1937, block for 700 ms. Run both through 1 s.
Depends on #8175. Review only this PR's change: vjeux/astryx@2c7fc52...6b391ad
User impact
Part of making Astryx match iOS under a finger (
spec:AST-094): #8175 added pull-to-refresh to Lab with a stand-in indicator. iOS's differs: larger spokes, a trail fading behind a clockwise head, springing in after the pull triggers. This PR draws iOS's on every touch device.Options and tradeoffs:
::afterwould tick Chromium's main thread).Visual authority
DDrow reused or amended.Design decisionstable by a team design owner.docs/design/.Settled by
spec:AST-094FR1–FR4, with UIKit's measured values. Under Reduce Motion the trail applies at once (design:reduced-motionDR2, DEC-3).Open with the owners: the brief copy (an INV7 exception), and the drawing under a fine pointer, which every pointer now pulls (FR43).
Before and after
Mobile Safari: the trigger, then a full pull:
UIKit, then this PR, mid-spring:
A 0.7 s busy script at the trigger, then mid-spin:
Android, then one turn in Chromium:
State matrix
Size (import gzip, #8175 → head): JavaScript 55,841 → 56,413 B (+572), CSS 621 → 962 B (+341),
lab.css11,527 → 11,865 B (+338).Scope
Testing
What I ran and what it showed: