Repository navigation
feat(core): Carousel's hasSnap takes useSnapPaging(), iOS paging on native snap's stops - #8153
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.
|
e092dd8 to
a7aadca
Compare
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Carousel · Storybook | 59.1 kB (+823 B) · 10.1 kB CSS (0) | 592 (+99) | ✓ · ✓ · ✓ |
| charts/Chart | 173.7 kB (0) · 494 B CSS (0) | — | — · — · — |
| charts/ChartTooltip | 151.1 kB (0) · 423 B CSS (0) | — | — · — · — |
Package size:
- @astryxdesign/cli 3.39 MB packed (+515 B); no new dependencies
- @astryxdesign/core 6.71 MB packed (+13.3 kB); no new dependencies
API changes: core +6 · ~1 · −0
Committed API report behind this build: cli 19 rows, core 21 rows; run pnpm api:report.
7 API changes (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Hooks · hook useSnapPaging |
hook useSnapPaging: (options?: SnapPagingOptions) => SnapPaging |
import {useSnapPaging} from '@astryxdesign/core'; useSnapPaging(…) |
| + | core |
. · Types · interface SnapPaging |
interface SnapPaging |
import type {SnapPaging} from '@astryxdesign/core' |
| + | core |
. · Types · interface SnapPagingOptions |
interface SnapPagingOptions |
import type {SnapPagingOptions} from '@astryxdesign/core' |
| + | core |
./SnapPaging · Also exported from an earlier entry point · SnapPaging |
SnapPaging (.) |
import {SnapPaging} from '@astryxdesign/core/SnapPaging' |
| + | core |
./SnapPaging · Also exported from an earlier entry point · SnapPagingOptions |
SnapPagingOptions (.) |
import {SnapPagingOptions} from '@astryxdesign/core/SnapPaging' |
| + | core |
./SnapPaging · Also exported from an earlier entry point · useSnapPaging |
useSnapPaging (.) |
import {useSnapPaging} from '@astryxdesign/core/SnapPaging' |
| ~ | core |
. · Components · component Carousel |
prop hasSnap?: boolean = false → prop hasSnap?: boolean | SnapPaging = false |
<Carousel hasSnap /> |
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
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.
This needs an engineering owner's decision before it can be approved. No current spec covers the new hasPaging prop, the touch pager that replaces native scrolling (draft spec:AST-094 itself says scrolling stays native) or the 0.3 s slide kept under Reduce Motion, and the new adaptive bottom-sheet threshold changes the width-and-pointer query that component:Selector DEC-1 keeps. An engineering owner records the decision, or the amendments it needs, by commenting /approve-spec <full head SHA>; the author may do so as an engineering owner. Whatever that decision is, please also:
- land or retarget the six predecessor PRs so this diff is the 1,269-line Carousel change rather than 29,083 lines;
- run the interaction proofs through Core's existing test lane instead of the separately routed
pr-interactionjob (spec:AST-030FR11); - get
lintpassing (ESLint runs out of heap) and fix the Paging stories' text contrast that failspr-a11y; - keep a single-pointer alternative to swiping when
hasPagingis used withhasButtons={false}.
[Automated review]
| * `hasSnap`, `hasEdgeFade` and `padding` do not apply. | ||
| * @default false | ||
| */ | ||
| hasPaging?: boolean; |
There was a problem hiding this comment.
hasPaging already names PageUp/PageDown focus paging in the exported useListFocus options (useListFocus.ts:95). architecture:public-component-api INV2 says names carry the same meaning across the public surface. Keeping this name is part of the owner decision the open question asks for.
There was a problem hiding this comment.
Left to the owner decision the description asks for (the public prop name).
| pagingTouchScroller: { | ||
| overflowX: 'clip', | ||
| overflowY: 'clip', | ||
| }, |
There was a problem hiding this comment.
On a coarse pointer this clip removes native scrolling, so the custom pan is the only touch path. With hasButtons={false} the edge buttons are never rendered (line 770), which leaves no single-pointer alternative to the path-based swipe (WCAG 2.2 SC 2.5.1, the spec:AST-020 FR1 baseline). Keep the buttons whenever touch paging is active, or fall back to native per-page snapping when hasButtons is false.
There was a problem hiding this comment.
Fixed in 226a949: with hasButtons={false} the carousel snaps natively on iOS too, so the browser's own scrolling pages it and no author gesture is the only way. Listed in the description as proposed, pending owner review. Test: "without the buttons it pages natively" (failed before).
| return undefined; | ||
| } | ||
| const gap = Number.parseFloat(getComputedStyle(track).columnGap) || 0; | ||
| pager.setOptions({gap}); |
There was a problem hiding this comment.
The pager gets the gap only here, at attach. This effect's dependencies omit gap, and usePager's measure() re-reads only the width. After a gap change, place() uses the live gap while the pager keeps the old stride, so page n rests n × the difference off. Re-send pager.setOptions({gap}) when the gap changes.
| useIsomorphicLayoutEffect(() => { | ||
| pager.setOptions({count: Math.max(1, count), loop: loops}); | ||
| if (isEnabled) { | ||
| place(); | ||
| } | ||
| }, [pager, count, loops, isEnabled]); |
There was a problem hiding this comment.
This Effect names no external system (architecture:react-component-runtime INV3). When count shrinks below the current page it sets page through the pager's clamp (setOptions → goTo → onIndexChange → setPage). Derive the clamped page during render, and add the comment naming the pager it synchronizes.
There was a problem hiding this comment.
Fixed in 226a949: the page is the pager's, read through useSyncExternalStore and clamped during render; the count Effect names the pager it synchronizes and reports nothing. Test: "a smaller count clamps the page shown, and growing back keeps the pager and the controls in step" (failed before).
| useEffect(() => { | ||
| if (index != null && index !== pager.index) { | ||
| pager.goTo(index, {reason: 'controlled'}); | ||
| } | ||
| }, [pager, index]); |
There was a problem hiding this comment.
A controlled index change calls the consumer's onIndexChange from this Effect (goTo → report). INV2 of architecture:react-component-runtime says the callback fires from the handler that caused the change, never from an Effect reacting to the resulting state. Skip the echo for reason: 'controlled', or name the external system this synchronizes.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
This job routes itself (pr-scope-touches.sh with its own path list) and is not part of the required test join. 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." Run these proofs under Core's existing test owner, or split the lane with an owner-approved AST-030 amendment.
There was a problem hiding this comment.
Not this PR's code: the pr-interaction job comes from the interaction-harness foundation below this commit, and is routed to its owners.
| export const COMPACT_TOUCH_QUERY = | ||
| '(width < 744px) and (pointer: coarse), (height < 500px) and (pointer: coarse)'; |
There was a problem hiding this comment.
This query becomes presentation="adaptive"'s for five components. It adds a height test to the released (max-width: 768px) and (pointer: coarse), while component:Selector DEC-1 says adaptive "keeps its width-and-pointer query". Phones in landscape now get the sheet, and 744–768 px portrait tablets get the popover. This waits on the owner decision in the open question.
There was a problem hiding this comment.
Not this PR's code: the adaptive query belongs to the touch-presentation change below this commit, and is routed to its owner.
| {title: 'Pack', body: 'A checklist for every trip.', color: '#5b8c5a'}, | ||
| {title: 'Go', body: 'Tickets and directions offline.', color: '#c46a3a'}, |
There was a problem hiding this comment.
pr-a11y fails on the three Paging stories with a serious color-contrast violation, two nodes each. The white 14 px body text measures 3.93:1 on #5b8c5a and 3.84:1 on #c46a3a, under WCAG's 4.5:1. Darken these two fills, or use a token pair that reaches 4.5:1.
There was a problem hiding this comment.
Fixed in 7fad86a: the Paging stories' fills are darker (white text reaches 4.5:1), and pr-a11y passes.
a7aadca to
7fad86a
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers the new hasPaging prop, the touch pager that replaces native scrolling, or the 0.3 s slide kept under Reduce Motion, and the draft spec:AST-094 it relies on is unapproved (its FR8 asks for a cross-fade where the code keeps the slide). An engineering owner records that decision, or the amendments it needs, by commenting /approve-spec <full head SHA>; the author may do so as an engineering owner. Whatever is decided, please also:
- land or retarget the six predecessor PRs so this diff is the 1,269-line Carousel change;
- move the adaptive bottom-sheet query change, which contradicts
component:SelectorDEC-1, into its own PR with an amendment to that decision; - run the interaction proofs under Core's existing test owner instead of the separately routed
pr-interactionjob (spec:AST-030FR11); - get the required
lintcheck passing (ESLint runs out of heap); - keep a single-pointer alternative to swiping when
hasPagingis used withhasButtons={false}.
[Automated review]
| const token = ++settleToken; | ||
| const handle = animator.play( | ||
| {x: toTrackCurve(curve, sign())}, | ||
| {reducedMotion: 'keep'}, |
There was a problem hiding this comment.
This keeps every pager move under Reduce Motion, including button, key, handleRef and focus moves that no finger drives. Draft spec:AST-094 FR8 in this same PR says such a move "fades through" and that only motion following a finger keeps moving. Whichever way the owner decides, make the code and FR8 agree, for example the animator's default 'crossfade' for goTo and 'keep' only for the settle after a drag.
There was a problem hiding this comment.
Left to the owner decision listed in the description (Reduce Motion keeps paging motion, as UIKit does; AST-094 FR8 follows the decision).
7fad86a to
fcf974c
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers the new hasPaging prop, the iOS pager that replaces native scrolling, the 0.3 s slide it keeps under Reduce Motion, or its theme-tunable paging motion, and the draft spec:AST-094 they rely on is unapproved (its FR4 and FR8 also disagree with the code). An engineering owner records that decision, or the changes it needs, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever is decided, please also:
- land or retarget the seven predecessor PRs so this diff is the 1,375-line Carousel change;
- move the adaptive bottom-sheet query, the role-keyed theme motion API with its
platformcondition, and the experimental subpaths into their own PRs with the amendments they need (component:SelectorDEC-1,architecture:theme-authoring-contract,spec:AST-017FR21); - run the interaction proofs under Core's existing test owner instead of the separately routed
pr-interactionjob (spec:AST-030FR11); - keep a single-pointer way to page when
hasPagingis combined withhasButtons={false}, and keep the pager's gap in sync with the track.
[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 platform axis to theme adaptation conditions, compiled to @supports (-webkit-touch-callout: none) in generateThemeRules.ts. architecture:theme-authoring-contract says “An adaptation change preserves the fixed condition vocabulary”, and the record is not amended here. Please move the role-keyed motion API and this condition to their own PR with that amendment.
There was a problem hiding this comment.
Not this PR's code: the platform adaptation condition belongs to the theme foundation below this commit, and is routed to its owner.
fcf974c to
226a949
Compare
|
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. |
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers the new hasPaging prop or the iOS pager that replaces native scrolling, the draft spec:AST-094 they rely on is unapproved, and the pager's 0.3 s slide and travelling settle under Reduce Motion contradict the current design:reduced-motion record (an opacity-only fade of 200 ms or less) unless that record is amended. An engineering owner records the decision, or the changes it needs, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever is decided, please also:
- land or retarget the seven predecessor PRs so this diff is the 1,501-line Carousel change;
- build the adaptive bottom-sheet query from the theme's compact definition (
spec:AST-012FR9: below themdpoint or under 500 px tall) instead of 744 px, in its own PR; - cap the animator's Reduce Motion cross-fade at 200 ms, and move the theme motion roles with their
platformcondition and the experimental subpaths into their own PRs with the amendments they need; - run the interaction proofs under Core's existing test owner instead of the separately routed
pr-interactionjob (spec:AST-030FR11), and addcomponent:CarouselFR2 and FR3 browser cases forhasPaging.
[Automated review]
| return; | ||
| } | ||
| follow( | ||
| animator.play({x: toTrackCurve(curve, sign())}, {reducedMotion: 'keep'}), |
There was a problem hiding this comment.
Under Reduce Motion this keeps the travel of the settle and of goTo (reducedMotion: 'keep', also at line 453). Current design:reduced-motion DR2 asks for an opacity-only fade of 200 ms or less on a coarse pointer and after a touch drag's release, DR4 rules out keeping a longer animation, and its DEC-1 rejected a per-component keep. Either an engineering owner amends that record for paging (the open question asks), or the settle and the moves conform.
| * Cross-Fade Transitions on (opacity CABasicAnimation 0.5 s, | ||
| * (0.42, 0, 0.58, 1)), Oct 23 2026 | ||
| */ | ||
| reducedMotionCrossFadeMs: 500, |
There was a problem hiding this comment.
This 500 ms is the default substituted fade for every animator move under Reduce Motion (animator.ts:1420). No core component plays it yet (the pager uses 'keep'), but current design:reduced-motion DR2 caps a touch fade at 200 ms, opacity only, and DR4 rules out a theme value that lengthens it, which defineTheme.ts:262 documents. Cap it at 200 ms and refuse longer theme values, or amend the record.
| export const TOUCH_QUERY = '(pointer: coarse)'; | ||
|
|
||
| /** The viewport is compact: layout behaviours. */ | ||
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; |
There was a problem hiding this comment.
Current spec:AST-012 FR9 defines compact as a width below the effective md point (768) or a height below 500, and says the web touch presentation resolver uses the theme's one definition, with no numbers of its own; main exports it as compactDefinition. With 744 here, a 744–767 px coarse-pointer screen (an iPad mini in portrait) opens the popover where FR9 gives the sheet, and a theme that moves md is ignored.
| // pager's swipe (WCAG 2.5.1), and the browser's own scrolling needs none. | ||
| const trackRef = useRef<HTMLDivElement>(null); | ||
| const {idiom} = useTouchPresentation(); | ||
| const isTouchPaging = hasPaging && idiom === 'ios' && hasButtons; |
There was a problem hiding this comment.
hasButtons={false} now also turns the iOS pager off. architecture:public-component-api INV3 (one prop, one concept) and INV16 (a presentation value keeps the fine-pointer input mode) bear on this coupling, and draft FR9 and DEC-2 reject native snapping on iOS; it is part of the owner decision the open question asks for.
| 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 exists at this head or on main, so the departure recorded here, and repeated under Current-state impact, cannot be weighed. Cite the record where it exists, or drop the reference until it lands.
|
Agreed: one implementation.
|
226a949 to
03cb63d
Compare
github-actions
Bot
added
the
needs:design-review
|
/approve-spec 8c3747a |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision, and one fix is needed.
- Decision:
useSnapPagingis a new public hook with no hook doc and no runnable example block that names it (architecture:knowledge-contractsINV28); an engineering owner needs to say whether it can ship with that gap tracked as an issue or must add them in this pull request. - Fix: the paging stories swap two
useSnapPaging()values through themodecontrol (apps/storybook/stories/Carousel.stories.tsx:509-511), and Carousel keeps the first value it attached (component:CarouselFR5), so switching betweenpaginganditemin the preview keeps the old page size; drive one hook'spagesfrommodeinstead.
Otherwise the change matches component:Carousel FR5-FR9: hasSnap alone stays native and bundles no pager code, and the touch, trackpad and bundle proofs pass on this head. CI has not run on this head yet.
[Automated review]
| const view = useSnapPaging(); | ||
| const item = useSnapPaging({pages: 'item'}); | ||
| const hasSnap = mode === 'snap' ? true : mode === 'item' ? item : view; |
There was a problem hiding this comment.
Carousel attaches only the first SnapPaging value it gets (component:Carousel FR5), so swapping view and item here leaves the first one attached: after switching the control from item to paging, a flick still moves one item. One hook whose pages follows mode fixes it, since the hook applies a new pages from the next flick:
const paging = useSnapPaging({pages: mode === 'item' ? 'item' : 'view'});
const hasSnap = mode === 'snap' ? true : paging;
8c3747a to
7fef456
Compare
github-actions
Bot
added
the
needs:design-review
|
/approve-spec 7fef456 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It runs once the pull request leaves draft and, because the branch is in a fork, a maintainer adds the ci:run label.
The change matches component:Carousel FR5–FR9: hasSnap alone stays native and bundles no pager code, the hook has its doc and a runnable example, and the touch, trackpad and bundle proofs pass on this head.
Optional wording fixes:
packages/core/src/Carousel/Carousel.tsx:143: thehasSnapJSDoc says the value pages "under a finger"; add the trackpad swipe, as the docs and changeset do.CarouselSnapPaging.doc.mjs:10: "each flick moves one screen" reads as one card, but the default moves one view (about two cards); say "one view".component:Carouselstill says snap paging is not built (its Compatibility row and FR7); those gap sentences and the new SnapPaging tests inverified_bycan follow.
[Automated review]
| /** | ||
| * Enable scroll-snap on items. Each direct child snaps to the start edge. | ||
| * Snap each item to the start edge. `true` (written `hasSnap`) uses native | ||
| * CSS scroll snap. A `useSnapPaging()` value adds iOS paging under a finger |
There was a problem hiding this comment.
Optional: a trackpad swipe pages too, under every pointer (as Carousel.doc.mjs and the changeset say). Suggest "adds iOS paging under a finger and on a trackpad swipe".
| name: 'useSnapPaging — Onboarding Pages', | ||
| displayName: 'useSnapPaging — Onboarding Pages', | ||
| description: | ||
| "Onboarding screens in a Carousel with hasSnap={useSnapPaging()}: under a finger or a trackpad swipe, each flick moves one screen and rests on it. A switch picks one screen or one card per flick through the hook's pages option.", |
There was a problem hiding this comment.
Optional: the cards are the onboarding screens, so "each flick moves one screen" reads as one card, while the default moves one view (about two of the 220 px cards here). Suggest "each flick moves one view" and "one view or one card per flick".
7fef456 to
6068fcd
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 6068fcd |
There was a problem hiding this comment.
Changes requested: snap paging matches the Carousel contract, and its unit, Chromium and WebKit tests pass on this head, but one fix is needed in how Carousel attaches it.
- Miss:
Carousel.tsxlines 408–415 attach and release the snap-paging value with their ownuseEffect; attach it through the shareduseAttachmenthook instead (for exampleuseAttachment(scrollElRef, scroller => attachedPaging == null ? () => {} : attachSnapPaging(attachedPaging, scroller), attachedPaging)), so the pager keeps one attach lifecycle that also follows a replaced scroll element.
Optional: the origin/travel comment in SnapPaging.ts could become names such as heldOffset and heldTravel.
[Automated review]
| // Syncs snap paging with the scroll area: it attaches to the element. | ||
| useEffect(() => { | ||
| const scroller = scrollElRef.current; | ||
| if (attachedPaging == null || scroller == null) { | ||
| return undefined; | ||
| } | ||
| return attachSnapPaging(attachedPaging, scroller); | ||
| }, [attachedPaging]); |
There was a problem hiding this comment.
Miss (blocking): this Effect is a second attach-and-release lifecycle for the pager. useAttachment (packages/core/src/hooks/useAttachment.ts) already owns that lifecycle for the gesture hooks, the pager and useAnimator, attaches in a layout effect, and follows a replaced node. Suggested:
useAttachment(
scrollElRef,
scroller =>
attachedPaging == null
? () => {}
: attachSnapPaging(attachedPaging, scroller),
attachedPaging,
);With this change the SnapPaging, Carousel and bundle tests still pass, and useAttachment imports no pager code, so FR8 holds.
| */ | ||
| function childTrack(element: HTMLElement) { | ||
| let isMoving = false; | ||
| // The scroll offset the take-over holds, and the row's travel then. |
There was a problem hiding this comment.
Non-blocking: names such as heldOffset and heldTravel would say what this comment says, and the comment could go.
|
/approve-spec 079514d |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet; it runs once the pull request is marked ready for review and, because the branch is in a fork, a maintainer adds the ci:run label.
hasSnap={useSnapPaging()} matches the current Carousel contract, and the unit, Chromium and WebKit tests pass on this head, with the built package keeping the pager out of a Carousel that is not given the hook's value. Carousel now attaches the value through the shared useAttachment lifecycle, which resolves the earlier request.
Optional: scripts/snap-paging-bundle.test.mjs bundles source, while the Carousel record's FR8 verification row asks for the built package, base against head; aligning one with the other would keep them in step.
[Automated review]
|
FR8's verification row now states what the bundle test does: Core's source with the size report's esbuild options, output attributed by module, and a positive control. Done in 5349630. |
|
/approve-spec 5349630 |
There was a problem hiding this comment.
Changes requested: snap paging for Carousel is well scoped, and every delta matches component:Carousel FR5–FR9, but one FR7 defect needs a fix first.
- Miss: under
hasSnap={useSnapPaging()}, pressing Scroll right (or callinghandleRef.scrollNext()) during the settle after a flick leaves the row resting between item edges (312–342 px against stops at 308 and 411 on the three-per-view story), because the step inCarousel.tsx:479-500reads the held scroll offset and the still-translated children, then races the hand-back'sscrollTo; hand the row back at its shown position before stepping, and add a browser case that presses the button mid-settle.
The rest checks out at this head, including the bundle boundary on the built package and the hook's doc and example. CI has not run yet because the pull request is a draft.
[Automated review]
| const at = Math.abs(el.scrollLeft); | ||
| const stops = snapStops( | ||
| el, | ||
| at, | ||
| Math.max(0, el.scrollWidth - el.clientWidth), | ||
| ); | ||
| const to = at + direction * Math.max(amount, itemWidth); | ||
| let next = stops.reduce( | ||
| (best, stop) => | ||
| Math.abs(stop - to) < Math.abs(best - to) ? stop : best, | ||
| stops[0], | ||
| ); | ||
| if (direction === 1 && next <= at + 0.5) { | ||
| next = stops.find(stop => stop > at + 0.5) ?? next; | ||
| } else if (direction === -1 && next >= at - 0.5) { | ||
| next = [...stops].reverse().find(stop => stop < at - 0.5) ?? next; | ||
| } | ||
| el.scrollTo({left: rtlSign * next, behavior}); |
There was a problem hiding this comment.
Miss (blocking): while a flick's settle runs, the row is taken over: el.scrollLeft still holds the offset the gesture started from, and the children still carry the settle's translate, so at and snapStops(el, at, …) here describe a position nobody sees. The smooth scrollTo below then races the hand-back's instant scrollTo (SnapPaging.ts:210). Pressing Scroll right 100–250 ms after a flick on the three-per-view story leaves the row at 312–342 px, between the stops at 308 and 411 (FR7), and handleRef.scrollNext() takes the same path. Hand the row back at its shown position through the value before stepping (for example a call next to attachSnapPaging that stops the settle where it shows), then read the offset and stops, and add a browser case that presses the button mid-settle.
There was a problem hiding this comment.
Done in d601a4c: while a flick holds the row, the edge buttons, scrollNext/scrollPrev, scrollTo and Shift+wheel move it with the paging's own animated scroll from where it shows, and native scrolling returns at rest; a browser case presses mid-settle and asserts the rest is a stop past the flick's own target.
…s a finger's pan (spec:AST-094 FR43) On the axis that a pan's derived touch-action keeps from the browser, the pan's own element is the pan's for a trackpad's scroll too, as it is for a finger. A scroller inside it still takes the wheel first, and a mouse wheel's notches stay native. The mechanism this needs: manageStyle: false applies to its own recognizer, so a pager's catch on the same element no longer drops the pan's touch-action. The gesture README and the allowedScrollTypes JSDoc state the own-scroller rule.
|
/approve-spec d601a4c |
There was a problem hiding this comment.
Changes requested on #8153; #10212 is approved pending CI. A pull request in this stack lands only when it and every pull request below it are approved.
#10212: approved pending CI. CI has not run on this head yet: it runs once the pull request is ready for review and, because the branch is in a fork, a maintainer adds the ci:run label. The own-scroller trackpad routing and per-recognizer manageStyle: false match spec:AST-094 FR41 and FR43, and the earlier README and attach-order requests are done.
#8153: changes requested. The press-during-settle defect from the last review is fixed: the row now rests on a stop in both directions.
SnapPaging.ts:345-413:moveTore-implements the pager'sgoTomove and its settle catch (a window pointer watch held forpaging.catchStopMs) instead of extendingcreatePager, so give the pager an offset move that runs as its own settle, and havemoveHeldmake that one call.
Whole stack: it fails on that #8153 item; the net API (hasSnap?: boolean | SnapPaging, useSnapPaging, the ./SnapPaging entry) and the gesture-host change otherwise agree with component:Carousel and spec:AST-094.
[Automated review]
| * A scroll call's move of the held row to `to`, as the pager's `goTo` plays. | ||
| * It replaces the settle, and hands the row back at rest. | ||
| */ | ||
| function moveTo(to: number): void { |
There was a problem hiding this comment.
Blocking (one shared path): moveTo repeats createPager's goTo move (solveAnimatedPageScroll from the shown position with the running velocity, usePager.ts:1107-1155), and the window watch below repeats the pager's settle catch (SettleCatcher and onCatch, usePager.ts:386-403 and :1069-1105). The pager's catcher is attached to this same row and fires on the same touch, but stops nothing, because this move is not its settle. The two already differ: a hold-then-lift after a flick rests through settleFrom, after an edge press through this animated scroll.
Suggest an offset move on the pager (for example goTo taking a position) that runs as its own settle, so its catcher stops it. moveHeld then makes that one call, and this curve code and the subscribeInput watch go.
There was a problem hiding this comment.
Done in 2dc6f90: goTo takes an offset, so the press move is the pager's own settle and its catch stops it; moveHeld makes that one call, and the curve code and the pointer watch are gone.
…ative snap's stops
hasSnap keeps native CSS snap. hasSnap={useSnapPaging()} pages the row
under a finger and on a trackpad swipe, for every pointer: a flick moves
one view, or one item with {pages: 'item'}, landing on an item's start
edge. A mouse wheel and a mouse drag keep native scrolling. Only a
carousel given the hook's value bundles the pager, and the hook has no
non-React equivalent. The hook has its doc page and a runnable example
block, and Carousel's record no longer lists snap paging as unbuilt.
|
/approve-spec 2dc6f90 |
|
/approve-spec d432fc9 |
There was a problem hiding this comment.
Changes requested: snap paging matches the component:Carousel contract, and the held-row move now runs through the pager's own goTo, which fixes the earlier copied move and catch. One fix remains:
- Miss: while a flick holds the row,
moveHeld(SnapPaging.ts:350) and Carousel'spagingStepand wheel listener (Carousel.tsx:335,:461) clamp to the livescrollWidth - clientWidth, which the children's translate widens after a flick back, so Scroll right from item 6 carries the three-per-view row 70 px past its last item before it jumps back at the hand-back, against FR7's "no empty space shows"; clamp to the travel the row took over with (heldTravel) and add a browser case that presses after a flick back near the end.
Two small non-blocking notes are in the review: pagingStep could reuse nearestStop, and FR8's "only calls attach and detach" sentence no longer describes the moveHeldSnapPaging calls.
[Automated review]
| } | ||
| // A finger on the row keeps it. | ||
| if (!pager.isDragging) { | ||
| const end = Math.max(0, element.scrollWidth - element.clientWidth); |
There was a problem hiding this comment.
Miss (blocking): while the row is held, its children carry a translate, and a translate toward the inline end widens the scrollable overflow. After a flick back from item 6 on the three-per-view story, scrollWidth reads 986 px against 916 px at rest, so this clamp lets Scroll right carry the row to 686 px against a 616 px end: 70 px of empty space after item 9, then a jump to 616 px at the hand-back (component:Carousel FR7: "the last stops clamped where the row ends so no empty space shows"). Clamp to the travel the track took over with (heldTravel, which stops() already uses), and pass that end to Carousel's pagingStep and wheel listener, which read the same live value (Carousel.tsx:335, :461).
There was a problem hiding this comment.
Done in fff7785: the held moves clamp to the travel the row was taken over with (heldTravel), passed to Carousel's pagingStep and wheel listener; a browser case presses after a flick back near the end and checks no space past the last item on every frame (70 px before the change). pagingStep reuses nearestStop, and FR8 now names the move call.
While a flick holds the row, its children's translate widens the row's overflow, so its live scroll width runs past its last item. The press, Shift+wheel and scrollTo moves now clamp to the travel the row was taken over with, so no space shows past the last item. Carousel's record states that it also asks the value to move a row it holds.
|
/approve-spec fdfb308 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The branch is in a fork, so a maintainer adds the ci:run label for CI to run.
The last review's blocker is fixed: held-row moves now clamp to the travel the row was taken over with, so Scroll right after a flick back near the end stops at the last stop with no space past the last item, and the new browser case covers it. Every delta matches the component:Carousel contract, and the FR8 wording is approved for this head.
[Automated review]
Rebuilt on main after #9666 merged; the gesture host's supporting fixes landed in #10212.
User need
A photo strip or onboarding row on a phone should page like a native paging scroll view: a flick moves one view or one item, resting on an item's start edge. Native CSS snap leaves the momentum to the platform, so a fast flick can skip several items, and a trackpad swipe glides on.
hasSnapcannot express that, and Carousel cannot carry the pager without every carousel bundling it.Problem and solution fit
hasSnap={useSnapPaging()}pages on native snap's own stops, under a finger and on a trackpad swipe, for every pointer. A flick moves one view, or one item withuseSnapPaging({pages: 'item'}), and at most one page from where the drag started.hasSnapalone is unchanged.scrollNext,scrollPrev,scrollToand Shift+wheel move it through the pager's owngoToto an offset, from where it shows; a finger catches that move as it catches a settle.Current specification
component:CarouselFR5–FR9 (merged #9666):hasSnaptakesboolean | SnapPaging; FR7's stops, paging rule and trackpad swipe under every pointer; FR8's module boundary. Percomponent:CarouselFR9 (merged #9666), the hook has no non-React equivalent, so this PR shipsuseSnapPagingonly.spec:AST-094FR3, FR6 and FR43 give the paging rule, the settle and the trackpad routing.Public delta and caller burden
hasSnap?: boolean | SnapPaging(wasboolean);useSnapPaging(options?: {pages?: 'view' | 'item'}): SnapPaging; the@astryxdesign/core/SnapPagingentry, also on the root.<Carousel hasSnap>. To page:<Carousel hasSnap={useSnapPaging()}>. No codemod:truekeeps today's native snapping.useSnapPagingpage and a runnable example block. Four stories (one item per view, three per view, a partial last page, right-to-left), each with amodearg forhasSnap,useSnapPaging()and{pages: 'item'}.Design and alternatives
During a gesture the pager moves the row's children by translate from the held scroll offset, and at rest it hands the offset back to native scrolling, so the row stays a real scroll container for the scrollbar, the keyboard and the wheel. The record rejected a separate paging prop and a non-React twin.
Evidence
Chromium and WebKit proofs under a finger and a trackpad swipe, red first where behavior changed; see Testing.
Scope
Testing
'', not'pan-y pinch-zoom');goToto an offset, a settle its catch stops, in place of a copy of that move and catch. A caught-and-lifted press move rests on the pager's nearest page, as a caught flick does; the previous head rested it on the nearest stop. The unit case "goTo with an offset ends there as a settle, and a still finger that catches it settles on the nearest page" fails when the offset is ignored, and so does the browser 40 ms case.preventDefault(), the same 616 px; with the wheel's direction flipped, the rest misses the stop nearest its travel. ThepreventDefault()evidence is Chromium's: this WebKit build does not scroll the row natively on Shift+wheel.hasSnapalone keeps native snapping under a trackpad swipe; the entry exports onlyuseSnapPaging.--cpus=4,CI=1,LP_NUM_THREADS=4, 1 worker) ×10, 50 passed each; the whole SnapPaging spec in WebKit, 29 passed; the SnapPaging and hooks specs on the pinned pair, the pager's included, 47 passed, 7 skipped (the trackpad cases); the pager's spec in WebKit, 22 passed, 3 skipped; Carousel's a11y contracts, 13 passed; core jsdom, 19,496 passed (716 files); the full node project, 16,737 passed, 2 expected failures, 71 skipped.CI=true pnpm lintpass. Prettier passes, except onCarousel.doc.mjs, which keeps main's own format. The changeset, knowledge, i18n, exports, use-client, Lab readiness and sync checks pass. The docsite generator's registry route check passes. The committed API report is main's plus this pull request's rows; a fresh regeneration differs from it only by main's own drift. Storybook builds.pnpm review:pre: 1,997 authored lines, one finding, on the hook's options layout effect, which the pre-review check is being aligned to exempt.preventDefault()does nothing); a separate Carousel fix covers it.component:Carouseldrops its two implementation gaps for snap paging, which this pull request builds, keeps its FR7 evidence gap and its edge-button gap, lists the snap-paging unit, browser and bundle tests inverified_by, names the bundle test's method in FR8's verification row (Core's source with the size report's esbuild options and a positive control), and FR8 states that Carousel also asks the value to move a row it holds. Prettier re-pads the requirement table's other rows (whitespace only).component:CarouselFR5–FR9;spec:AST-094FR3, FR6, FR43;architecture:public-component-apiINV22;architecture:knowledge-contractsINV28, INV29;architecture:react-component-runtimeINV3, INV13.Writing rubric results
Over the
component:Carousellines this pull request changes:verified_bynames the three test files, and FR8's verification row states the bundle test's actual method; no pull request or commit is cited.