Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the detailed evidence. This needs an engineering-owner decision before it can be approved: no current spec covers hiding the dividers beside a touch-pressed List row, and the draft spec:AST-094 this branch adds (FR1–FR23, DEC-1, DEC-2) proposes amendments to current records that have to be approved or rejected. An engineering owner records that decision by commenting /approve-spec followed by this head's full commit SHA; you can do that yourself as an engineering owner, or the divider behavior can first be written into the List component spec. Separately, the diff should shrink to the 321-line divider commit by landing the predecessor PRs first (it is 12,777 authored lines today).
- AST-094 FR7 cites
spec:AST-090, which does not exist onmainyet.
[Automated review]
| 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.
There is no spec:AST-090 record on main or at this head, so a reader cannot see the exit rule FR7 departs from. Cite it once it lands, or state that rule here in its own words.
| * tap-to-click, long press, double tap and the start of a page scroll, so | ||
| * many guesses can be settled by measuring Safari itself. | ||
| * | ||
| * SYNC: When modified, update packages/core/src/gestures/README.md (the |
There was a problem hiding this comment.
packages/core/src/gestures/ is not in this tree: neither the gesture system line 7 mentions nor this README exists yet. Point the note at what exists, or land it with the recognizers.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsItem (@astryxdesign/core) · View in Storybook
TreeList (@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: 2 accessibility violation(s) found — 2 serious. Radio group pattern - 1 issue(s)
RadioList - 1 issue(s)
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 |
504873a to
0d02b37
Compare
There was a problem hiding this comment.
This needs an engineering-owner decision before it can be approved: no current spec covers hiding the dividers beside a touch-pressed List row, and the draft spec:AST-094 this branch adds (FR1–FR23, DEC-1, DEC-2) proposes amendments to current records that have to be approved or rejected. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and you can do that yourself as an engineering owner; otherwise the divider behavior can first be written into the List component spec, with AST-094 settled in its own spec PR. Separately, the diff should shrink to the 321-line divider commit by landing the predecessor PRs first: it is 13,697 authored lines today.
- AST-094 FR7 cites
spec:AST-090, which does not exist onmainyet, and a few motion-table notes point at files this branch does not contain (packages/core/src/gestures/,uikit/switch.ts,uikit/contextMenu.ts,sources.test.ts).
[Automated review]
0d02b37 to
c2edd43
Compare
There was a problem hiding this comment.
This needs an engineering-owner decision before it can be approved: no current spec covers hiding the dividers beside a touch-pressed List row or moving the touch press release of interactive Item and TreeList rows onto a compositor layer, and the draft spec:AST-094 this branch adds (FR1–FR23, DEC-1, DEC-2) proposes amendments to current records that have to be approved or rejected. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and you can do that yourself as an engineering owner; otherwise the divider behavior can first be written into the List component spec, with AST-094 settled in its own spec PR. Separately, the diff should shrink to the 500-line divider commit by landing the predecessor PRs first: it is 14,032 authored lines today.
- AST-094 FR7 cites
spec:AST-090, which does not exist onmainyet, and a few motion-table notes point at files this branch does not contain (packages/core/src/gestures/,uikit/switch.ts,uikit/contextMenu.ts,sources.test.ts). - The
pr-a11yfailure comes from the branch predating amaintest file (ReorderGripPinchZoom.a11y.chromium.spec.ts); rebasing ontomainclears it.
[Automated review]
c2edd43 to
01ce8b1
Compare
… that finger until it lifts (spec:AST-094 FR14) A recognition that claimed its touches and was then cancelled gave them back to the browser unless another recognition owned them, so a finger resting there could pinch-zoom the page. A gesture that cancels itself on a finger's move, a long press dragged past its tolerance after it began, gave back a finger that was dragging: the host stopped cancelling its moves and the page scrolled the rest of the drag, a hand change FR14 forbids and UIKit never makes (its scroll view never takes a touch another recognizer had). A cancel during a finger's move now keeps that finger claimed until it lifts; a resting finger is given back as before, and a waiting recognition that fails still gives its claim back. The new arena case fails on the previous code: the dragging finger reads unclaimed right after the cancel.
…edge-zone strip decides at UIKit's 15 px The host's arena environment had no viewSize, so under the real host a screen-edge pan never saw its edge zone while a touch was down and decided at 10 px, while its attach-time touch-action (read through the default environment) had already claimed the axis (spec:AST-094 FR18). Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
…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>
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.
…w press release change
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.
…(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
… rows united) into the row press release change
There was a problem hiding this comment.
Approval waits on an engineering-owner decision: no current spec covers hiding a List row's dividers under a touch press (component:List covers only the edge treatments, and design:user-states lists no list rows), and the open change request pauses this pull request until that is settled. An engineering owner can provide or identify the governing spec, or record approval of this behavior for this exact head by commenting /approve-spec 2121d8f641730a9d9c9adf7a1b0002ba44a74fdf; the author may do so as an engineering owner. Whatever the decision:
- Land #8076 and #8077 first, then rebase, so this pull request carries only the 671-line divider change instead of 18,311 authored lines.
- Report component sizes against this pull request's base (after the rebase, the
ListItemfigures are the whole report), and point the press-layer comments inListItem.tsxand theListDividersspec header atutils/rowPressLayer.ts.
[Automated review]
| borderBlockEndColor: { | ||
| default: colorVars['--color-border'], | ||
| '@media (pointer: coarse)': { | ||
| default: null, | ||
| '[data-astryx-press="on"]': 'transparent', | ||
| ':has(+ [data-astryx-press="on"])': 'transparent', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Under a coarse pointer this hides the pressed row's divider and the one above it. No current spec covers divider behavior during a press (component:List covers only the edge treatments, and design:user-states lists no list rows), so it needs an engineering owner's decision for this head before it can be approved.
| // While its divider is hidden, the press layer (the `cell` style of | ||
| // utils/interactionOverlay.stylex.ts) reaches over it, so the whole row |
There was a problem hiding this comment.
The layer this stretches is rowPressLayerStyles.cell in utils/rowPressLayer.ts; utils/interactionOverlay.stylex.ts has no cell. The ListDividers spec header (L13–14) points at the same wrong module.
|
Posted by Cindy's agent on her behalf. Cindy decided to keep today's pressed overlay with no new press treatment, so "dividers hide on press" won't land. Could you close this PR, and rebase the PRs stacked on it (#8082, #8085, #8106, #8359, and the #8139 line) off it? If that's not done by about 10:56 PT, we'll add one revert commit (behavior, changeset, tests and story) at the lowest PR stacked on this one so the rest of the line can move, keeping your authorship. If a theme-owned pressed treatment is wanted later, it can come back as its own proposal. |
|
Posted by Cindy's agent on her behalf. Correction to my comment above: I was wrong that an earlier decision rules this out. That decision covered a different control. Whether List dividers hide next to a pressed row is a new question for Cindy, and it's pending her decision. Please don't close or rebase anything for now; nothing will be reverted until she decides. Sorry for the noise. |
|
Posted by Cindy's agent on her behalf. Thanks for the iOS research here. Cindy decided to keep List dividers drawn while a row is pressed (today's behavior), with no new theme setting for hiding them. Could you close this PR and rebase the PRs stacked on it (#8082, #8085, #8106, #8359 and the #8139 line) off it? That includes #8082's selection hold, which also hides the dividers; that part should go too. If this stays idle for about 30 minutes, we'll add one revert commit at the lowest PR stacked on this one, covering behavior, changeset, tests and story, so the rest of the line can move. Your authorship stays. |
|
Posted by Cindy's agent on her behalf. One more ask while you rebase: please also move #8082 (the selection hold) and #8114 (NavigationStack's use of it) off the middle of the line and onto the top of the stack. Whether a tapped row stays selected through a navigation is still an open question for Cindy. With those two on top, the rest of the line (#8085, #8106, #8359, #8139 and the reorder PRs) can land without waiting on it. |
|
Closing per Cindy's decision: List dividers stay drawn while a row is pressed. Thanks for the review; the branch stays. |
Per the maintainer's decisions on meta#8079 and meta#8082: List dividers stay drawn while a row is pressed, with no theme setting, and a row's selection through a navigation stays the product's call through ListItem's existing `isSelected`, so Core gains no selection hold. This removes, from the touch row target change, everything meta#8079 and meta#8082 brought into it: the divider arms under a touch press, the press layer's reach over the divider, the hold and its export, their changesets, tests and stories. The touch target itself is unchanged. Two test helpers the touch target's own tests read stay: the story opener under a finger (touchStory.ts) and the rules split by the touch gate in List.test.tsx. Red first: the build-css test that every press state leaves the dividers drawn fails on the change before this one.
…his branch meta#8082 (a pushed row stays selected until the pop) and meta#8079 (a touch press hides the dividers beside the row) are closed, so their code leaves this branch: the selection hold and its tests, the divider hiding and its tests, their changesets, and their edits to List, Item, TreeListItem, the motion table, the row press layer and the coverage and CSS tests, each back to meta#8077's text.
…le uses From review on meta#8085: the exemption let a file that paints a press pass the guard whenever it rendered a marked component through `xstyle`. It came with ListItem's divider arms, which the revert of meta#8079 took out, so no file at this head reaches it, and in place it would wave through a file whose press styles never reach that component's root. The guard is again exactly as on the line below; it passes without the exemption.
Review only this PR's change: vjeux/astryx@01b48e5...2121d8f (depends on #8077; the same change also rides in #8082).
User impact
Pressing a row of a divided
List(hasDividers) under a finger: the dividers above and below stayed drawn across the highlight, cutting it with two hairlines. Now both hide while the press is on and return at the lift while the highlight fades, as a highlighted UIKit cell's separators do.Under a coarse pointer only (
spec:AST-094FR13): an Android finger gets the same, and a mouse keeps today's dividers.Size (gzip, the size report's tool, base → head): ListItem JS +34 B and CSS +79 B.
Visual authority
Reference: iOS 26.2
UITableViewandUICollectionViewlists, where a highlighted cell hides its own separator and the one above it, and both return in the first frame of the deselect.Before and after
The pressed row's edges, before | after (Chromium under a finger, ×4, contrast stretched): the divider crossed the press as a lighter seam; now the highlight is one band.
iOS 26.2 Safari (simulator, real touch):
Android 15, Chrome 124 (emulator): the same two hide, and return at the lift.
State matrix
Scope
Testing
Red first: an Android finger's press hides the same two dividers (browser, one touch model).
Green on #8076 3fb31dd and main 6aea9bd: root build and
pnpm typecheckon this head and its List, Item and TreeList suites (275); the stack's top, which carries it, passes theuiproject's 16,168 tests (load timeouts pass alone), Chromium 91 and WebKit 74.