Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
|
AI review status for this pull request.
|
![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's spec decision, and two fixes are needed.
Decision needed: this head rewrites component:Carousel FR8 so Carousel's scroll calls move the row through snap paging from rest, and it makes the edge fades follow a held row, which the Carousel record leaves uncontracted. An engineering owner records the decision by commenting /approve-spec 0c9ffa5f4a6bca78a02989a423fbe97cf3c99e60 on this pull request (the author may do so as an engineering owner), since the approvals on #10359 and #10384 named those pull requests' heads.
Blocking:
- In the nine-item three-up story, a row held at its end that narrows 100 px under the finger still rests at 616 of 716 px after the lift, because the release counts its one-page limit from
dragStartIndex(usePager.ts:985), an index into the page list from before the resize; map it to the page nearest the old start offset whenmeasure()replaces the pages during a drag, and add that story to the new test. span()(SnapPaging.ts:191-196) copies the row-scale read fromsnapStops(snapPagingValue.ts:146-151), so the held travel and the stops can drift apart; share one helper.
This branch also carries #10359 and #10384, so merging it first lands their changes too.
[Automated review]
0c9ffa5 to
d17ff2e
Compare
|
/approve-spec d17ff2e |
github-actions
Bot
removed
the
needs:spec-owner-review
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 a maintainer adds the ci:run label.
An engineering owner approved this head's spec content (the component:Carousel FR8 rewrite and the edge fades following a held row), and the two blocking findings raised earlier are fixed: a row held at its end that narrows by 100 px in the three-per-view story now rests at the row's end, and the span and the stops share one scale read.
- Non-blocking: the existing test title "the scroll offset holds through a swipe, so the edge fades and buttons stay put" (
SnapPaging.interaction.browser.spec.ts:351) now contradicts the edges following a held row; consider "the scroll offset holds through a swipe, and the settle hands it on".
This branch also carries #10359 and #10384, so merging it first lands their changes too.
[Automated review]
d17ff2e to
1e78835
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 1e78835 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision for this head: the component:Carousel record changes this branch carries (FR8's rewrite and its held-row sentence, the edge-fades line, and the FR1/AR1 and FR8 verification rows) have no approval for it, because the earlier /approve-spec here named a head whose record carried only the FR8 rewrite. An engineering owner records the decision by commenting /approve-spec 1e788353d598ac671b3f6cec4c730fb2d32a7aa5 on this pull request, and the author may do so as an engineering owner. The held-resize fix itself matches FR7, and both earlier blocking findings are fixed: a row narrowed 100 px under a finger now rests at 716 of 716 px in the three-per-view story, and the span and the stops share one scale read.
- Non-blocking: two new comments in
SnapPaging.ts(lines 178 and 385) restate a name; see the inline suggestions. - CI has not run on this head, and this branch also carries #10384, so merging it first lands that pull request's changes too.
[Automated review]
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 a maintainer adds the ci:run label.
The engineering-owner /approve-spec for this head settles the component:Carousel record changes this branch carries, and the held-resize fix matches FR7: a row narrowed 100 px under a finger now rests at 716 of 716 px in the three-per-view story, where main rests at 616.
- Non-blocking: the two comment suggestions in
SnapPaging.ts(lines 178 and 385) still apply. - This branch also carries #10384, so merging it first lands that pull request's changes too.
[Automated review]
A held row keeps the travel it was taken over with, since its translate widens the overflow. A resize during the hold changed the true travel, but the last stops still clamped to the old one, so the lift rested short of the end with the last item cut off. The track now keeps the children's span and the row's width from the takeover, and the pager's measure moves the held travel by their change. When new pages arrive during a drag, the drag keeps the page nearest where it began, since its release moves at most one page from there. The stops and the span share one scale read.
1e78835 to
2c75272
Compare
|
/approve-spec 2c75272 |
There was a problem hiding this comment.
Changes requested: the narrowing fix works, but widening under a held finger now regresses.
- Miss: when the row widens under a finger held at its end, the browser clamps the held scroll offset while
remeasure()(SnapPaging.ts:197-211) also lowers the travel, so the settle ends 100 px short with the last item cut off and then jumps 100 px at the hand-back, wheremainrests at the end; re-read the native offset inremeasure()and add a widening case beside the new narrowing test. - Miss: the changeset says "a row that resizes while a finger holds it now rests where the row ends", but only narrowing is fixed and tested, so match the wording to what the fix proves.
The narrowing fix and the drag's start-page remap fail on main and pass here in Chromium and WebKit. CI has not run on this head yet.
[Automated review]
| heldTravel = Math.max( | ||
| 0, | ||
| Math.round(heldTravel + (nextSpan - heldSpan) - (nextWidth - heldWidth)), | ||
| ); |
There was a problem hiding this comment.
Miss: when the row widens under a finger held at its end, the travel falls below the held scroll offset and the browser clamps the row's scrollLeft (616 to 516 px in the three-per-view story), but heldOffset stays 616. Lowering heldTravel here as well places the last page 100 px short: the settle ends with item 9 cut off by 99.9 px, then the hand-back jumps 100 px to the end. main rests at the end with no jump (Chromium and WebKit). Re-read the native offset here and move heldOffset and the children's translate by any clamp, and add a widening case beside the new narrowing test.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Carousel paging: a row that resizes while a finger holds it now rests where the row ends, instead of short of its end by as much as it narrowed. |
There was a problem hiding this comment.
Miss: "a row that resizes while a finger holds it" covers widening too, which the tests do not prove and which now settles short and jumps. Either fix widening or say "a row that narrows while a finger holds it".
User impact
With snap paging, a Carousel row that narrows under a finger rests short of its end after the lift, its last item cut off: by 0.6 px after a 1 px resize, by 99.9 px after 100 px with three items per view.
Problem and solution fit
Now the held travel follows a resize, and a drag keeps its start page.
Expected behavior and authority
component:CarouselFR7: snap paging's last stops are "clamped where the row ends so no empty space shows"; its representative state: "The last page rests where the row ends." No record changes.Smallest restoration
childTrackreads the children's span and the row's width at the takeover;remeasure(), first inmeasurePages, moves the travel by their change. The span sharessnapStops' scale read,rowScale.usePager'smeasure()mapsdragStartIndexto the page nearest its old offset when the pages change during a drag.Evidence
iOS 26.2 simulator, Mobile Safari.
Before, takes rest at 411 of 412 or 616 of 716; after, at the end. Frame gaps ≤ 25 ms.
Scope
Testing
pnpm exec vitest run --project ui packages/core/src/{Carousel,SnapPaging,hooks,gestures}: 96 files, 1,930 tests pass.tscfor core, Prettier, ESLint,pnpm check:changesetsandpnpm check:knowledge: clean.