Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
f912e37 to
d8e3297
Compare
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is on hold for an owner decision: the TreeList touch expansion, its new followingContent prop, the transient translate TreeList writes onto page content below it, and the rest of the touch stack (List row heights, holdSelection, the adaptive threshold) have no current spec, and spec:AST-094 is still a draft. An engineering owner records that decision by commenting /approve-spec <full head SHA> once the records describe these behaviors (the author may do so as an engineering owner), and independently of it the change needs these fixes:
- Merge the predecessor PRs first so this one is only its two TreeList commits, since the diff now carries 15,771 authored lines.
- In
useTreeExpansionMotion, add an effect cleanup so an unmount mid-collapse cannot leave page content translated, and release each row's inlinetranslatewhen its own spring settles.
[Automated review]
a448c3b to
02811f3
Compare
02811f3 to
4f2fd44
Compare
There was a problem hiding this comment.
Approval is on hold for an owner decision: the TreeList touch expansion, its new followingContent prop, the transient translate it writes onto page content outside the tree, and the rest of the touch stack (List row heights, holdSelection, the adaptive threshold) have no current spec, and spec:AST-094 is still a draft. An engineering owner records that decision by commenting /approve-spec <full head SHA> once the records describe these behaviors, including whether the slide stays under Reduce Motion (the author may do so as an engineering owner). Independently of that decision, the change needs these fixes:
- Land the predecessor PRs first so this one is only its two TreeList commits; the diff now carries 16,735 authored lines.
- In
useTreeExpansionMotion, return an effect cleanup so an unmount mid-collapse cannot leave page content translated, and release each element's inlinetranslatewhen its own spring settles. - Move content below the tree by its measured layout change instead of the group height, so content layout does not move (a footer below a taller column) stops sliding.
[Automated review]
4f2fd44 to
bea7d0f
Compare
There was a problem hiding this comment.
Approval is on hold for an owner decision: the TreeList touch expansion, its new followingContent prop, the transient translate it writes onto page content outside the tree, and the rest of the touch stack (List row heights and dividers, holdSelection, the adaptive threshold) have no current spec, and spec:AST-094 is still a draft. An engineering owner records that decision by commenting /approve-spec <full head SHA> once the records describe these behaviors, including whether the slide stays under Reduce Motion (the author may do so as an engineering owner). Independently of that decision, the change needs these fixes:
- Land the predecessor PRs first so this one is only its two TreeList commits; the diff now carries 17,240 authored lines.
- In
useTreeExpansionMotion, return an effect cleanup so an unmount mid-collapse cannot leave page content translated, and release each element's inlinetranslatewhen its own spring settles. - Move content below the tree by its measured layout change instead of the group height, so content that layout does not move (a footer below a taller column) stops sliding.
- In
spec:AST-094, fix FR7's reference to an AST-090 record that does not exist in the repository.
[Automated review]
bea7d0f to
c1d18da
Compare
There was a problem hiding this comment.
Approval is on hold for an owner decision: the TreeList touch expansion, its new followingContent prop, the transient translate it writes onto page content outside the tree, and the rest of the touch stack (List row heights and dividers, holdSelection, the adaptive threshold) have no current spec, and spec:AST-094 is still a draft. An engineering owner records that decision by commenting /approve-spec <full head SHA> once the records describe these behaviors, including whether the slide stays under Reduce Motion (the author may do so as an engineering owner). Independently of that decision, the change needs these fixes:
- Make
HoldCurveTiming'scurveoptional, or pass a curve from the new "Back, slowly" story button, so the Storybooktypecheckcheck and thebuildgate pass again. - In
useTreeExpansionMotion, return an effect cleanup so an unmount mid-collapse cannot leave page content translated, and release each element's inlinetranslatewhen its own spring settles. - Move content below the tree by its measured layout change instead of the group height, so content that layout does not move (a footer below a taller column) stops sliding.
- Land the predecessor PRs first so this one is only its two TreeList commits; the diff now carries 17,251 authored lines.
- In
spec:AST-094, fix FR7's reference to an AST-090 record that does not exist in the repository.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about component:TreeList FR8 as this pull request amends it: under a finger and without an animatedLayout handle, the rows after a closing folder and the content after the tree hold until its rows have slid under it (about 0.3 s) instead of taking their places at once, and FR8 itself is still marked proposed. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner). Whatever the ruling, these changes are needed:
- Land #8360, #8890, #8077 and #8085 first, so this pull request carries only its 15 TreeList files instead of 34,186 authored lines.
- Play the outline spring through a motion role, as
spec:AST-094FR4 requires, instead of the internallist.update.outlineSpringconstant. - Drop the checkbox example from the press-release changeset (#8890): CheckboxInput's release runs on its row, not inside its box.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about component:TreeList FR8 as this pull request amends it: under a finger and without an animatedLayout handle, the rows after a closing folder and the content after the tree hold until its rows have slid under it (about 0.3 s) instead of taking their places at once, and FR8 itself is still marked proposed. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner). Whatever the ruling, these changes are needed:
- Land #8360, #8890, #8077 and #8085 first, so this pull request carries only its 15 TreeList files instead of 34,285 authored lines.
- Play the outline spring through a motion role, as
spec:AST-094FR4 requires, instead of the internallist.update.outlineSpringconstant.
[Automated review]
|
/approve-spec 2581b03 |
There was a problem hiding this comment.
Changes requested. The touch expand itself is settled: an engineering owner approved component:TreeList FR8 as this head words it. Four things still block it:
component:TreeListAV5 and the "Item toggled by a finger" state still say a touch collapse fades out under Reduce Motion or slides back, while the amended FR8 and the new browser case remove the group at the tap; reword both rows to match FR8.motion/themeTiming.tscopies the privateThemeTiming,themeTimingOfandtimedCurveinSegmentedControl/useSegmentedSelectionMotion.ts, which stay behind; have SegmentedControl import the shared module and delete its copy.- Desktop behavior change needs an owner decision: under Reduce Motion the Chevron now lands at once for a mouse as well (
TreeListItem.tsx:275-278); gate it to touch input, or cite the decision. - FR8's verification row requires a case that fails an animated property under a fine pointer with Reduce Motion, and the only Reduce Motion case runs under a coarse pointer; add a mouse case under Reduce Motion.
CI skipped its jobs on this head, so the new cases have not run yet, and any new commit needs the spec approval recorded again on its head.
[Automated review]
|
/approve-spec 54e08f1 |
There was a problem hiding this comment.
Changes requested. The touch expand itself is settled: an engineering owner approved component:TreeList FR8 and its companion rows as this head words them. Two things still block it:
- Regression: the changeset still says the chevron lands at its new angle at once under reduced motion "for every pointer", but the coarse-pointer gate in
TreeListItem.tsx(and your own mouse case) keeps the mouse's turn, so reword it to "With reduced motion on and a finger, the children fade in where they land and the chevron lands at its new angle at once. Trees driven by a mouse are unchanged." readThemeTimingand SegmentedControl'sthemeTimingOfstill read--duration-fastand--ease-standardtwo ways and already disagree on a value only CSS resolves (with--duration-fast: calc(400ms)the rows slide for 175 ms while the chevron turns for 400 ms), so keep one reader that resolves the tokens as CSS does and delete the other, here or by sitting this branch on the groundwork pull request you describe.
CI skipped its jobs on this head, and a new commit needs the spec approval recorded again on its head.
[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 leaves draft and a maintainer adds the ci:run label, since the branch comes from a fork.
The touch expand is settled: an engineering owner approved component:TreeList FR8 and its companion rows as this head words them, and the head's TreeList and theme-timing browser specs pass in Chromium and WebKit. Both earlier blockers are fixed: the changeset now limits the Reduce Motion chevron change to a finger, and SegmentedControl reads its timing through the one shared readThemeTiming, so a calc() token times TreeList's slide and its chevron alike.
This branch carries #10068's theme-timing commit, so #10068 lands first, as the description says.
Non-blocking:
SegmentedControl.interaction.browser.spec.ts:2314:openWithMouse's doc line now sits aboveclickSlide's doc block; move it back aboveopenWithMouse.- Under Reduce Motion with a mouse the chevron still turns, as it does on
main(design:reduced-motionDR3); fixing that is a separate desktop change.
[Automated review]
There was a problem hiding this comment.
Changes requested: one wording fix. The touch expand itself is settled: an engineering owner approved component:TreeList FR8 and its companion rows as this head words them, and the head's TreeList browser specs pass in Chromium and WebKit.
- Miss:
.changeset/tree-list-expand-motion.mdand theTreeList.doc.mjsdescription say that under a finger every tree slides a folder's children out and the rows below take their places at once, but the slide is off when a tree has ananimatedLayouthandle (TreeListItem.tsx:443), where the content after the row still glides and a collapse keeps its exit, so please begin both sentences with "Without ananimatedLayouthandle,".
Non-blocking: under Reduce Motion with a mouse the Chevron still turns, as it does on main; changing that is a separate desktop decision.
CI has not run on this head yet; it runs once the pull request leaves draft and a maintainer adds the ci:run label.
[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 leaves draft and a maintainer adds the ci:run label.
The changeset and the TreeList docs now scope the touch slide to trees without an animatedLayout handle, which was the one remaining change. The touch expand, the collapse at the tap and the Reduce Motion fade match component:TreeList FR8 as an engineering owner approved it, and the new Chromium and WebKit proofs pass on this head.
- Optional:
rowPixels.tsandreducedMotion.tssay they are shared with the List specs, butListRowPress.interaction.browser.spec.tskeeps its own copy of the beacon and WebKit video decoder; moving that proof onto the helpers, or correcting the two headers, keeps the copies from drifting.
[Automated review]
There was a problem hiding this comment.
Changes requested: one test fix. The touch expand itself is settled: an engineering owner approved component:TreeList FR8 and its companion rows as this head words them, and the TreeList browser proofs pass in Chromium and WebKit on this head. CI has not run yet; it runs once the pull request leaves draft and a maintainer adds the ci:run label.
- Regression: since
mainwas merged in,useTreeExpansionMotion.test.tsx:408fails, because theanimatedLayoutglide now reads the theme's move curve throughreadThemeTiming(motion/animatedLayout.ts:140) and the test's mock records every caller, so assert only that the row's own group read no timing (for exampleexpect(timed).not.toContain(group())) instead ofexpect(timed).toEqual([]).
Optional, carried from the last review: rowPixels.ts and reducedMotion.ts say they are shared with the List specs, but ListRowPress.interaction.browser.spec.ts keeps its own beacon and video decoder.
[Automated review]
|
/approve-spec 32687bb |
There was a problem hiding this comment.
Changes requested: one comment. The touch expand itself is settled: an engineering owner approved component:TreeList FR8 and its companion rows as this head words them, and the TreeList browser proofs pass in Chromium and WebKit. CI has not run yet; it runs once the pull request leaves draft and a maintainer adds the ci:run label.
- Newly live check:
useTreeExpansionMotion.ts:70readsgroup.offsetHeightwith no comment naming its reason, whicharchitecture:react-component-runtimeINV18 requires of every remaining layout read, so add one line above it such as// The block travels the group's height: one read per expand, before any write.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on this head's spec changes: component:TreeList FR8 (the touch glide inside the tree, including the children's clip-path reveal, which spec:AST-094 FR5 does not list) and DD5, and spec:AST-105 FR4 and DEC-3 (every layout glide moves to --duration-medium). An engineering owner records that decision by commenting /approve-spec 47a76c9cf18f3fa8f84b2a8dfcf6e1da506175ee on this pull request, and the author may do so as an engineering owner.
These need fixing either way:
- The guides now span only their own row, so when a parent is the last row at its level and an expanded child is not its last, the parent's guide line stops at that child (with a mouse, at rest, the Themed guides story breaks the line from
srctopackage.json); please draw each ancestor column from theisLastof the row whose connector sits there and add a browser case for that tree shape. - Desktop behavior change needs an owner decision:
motion/animatedLayout.ts:141lengthens every layout glide from 225 ms to 300 ms for mouse users too (AppShell's rail collapse, Collapsible, Table row expansion, aTreeListwith a handle), so please gate it to touch input, cite the decision in the description, or land it through #10261 first. - The description still describes the earlier slide and a collapse that removes the group at the tap, while this head glides the rows below and fades the group out, inert, as they glide up; please update the user need, design, evidence and testing sections to match.
In Chromium the touch glide itself behaves as the proposed FR8 describes, including the 200 ms cross-fade under Reduce Motion and content after the tree moving at once.
[Automated review]
| /** | ||
| * The clip a tree with no handle takes while it moves. Rows that glide up into | ||
| * a shorter tree then show as they enter it. So they never paint over the | ||
| * content after it, as in a table view that clips to its bounds. Only the | ||
| * bottom edge clips, past the reach of a row's focus ring. | ||
| */ |
There was a problem hiding this comment.
Non-blocking (comment style): "So they never paint over the content after it, as in a table view that clips to its bounds." is a fragment. Suggested: "Rows that glide up into a shorter tree show only inside it, so they never paint over the content after it. Only the bottom edge clips, past the reach of a row's focus ring."
A block edge's reveal or conceal clipped the element with a moving clip-path. WebKit paints that clip on the main thread while the content beside it glides on the compositor, so on iOS the two parted for a few frames. The element's child elements that the band can show now slide on the glide's frames under a still clip on its box, and the two share one clock. An element with text of its own keeps the clip. Collapsible's region wraps its children in one element so they slide. The glide's timing and curve are unchanged.
The default panel row slides its cell, and one that paints its own box keeps the moving clip. Table's record scopes its gap to the muted panel.
The panel's divider offset was read after its entrance had started, when its cell had already slid up by the panel's height, so the line hung above the panel's clip for the whole glide. The offset is now read with the panel's other borders, before its entrance moves its cell.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on this head's spec changes: component:TreeList FR8 (the touch glide and slide inside the tree) and DD5, and the records this branch brings from #10296 (spec:AST-105 FR9's block-edge slide, architecture:react-component-runtime INV12's child walk, component:Collapsible AV5 and component:Table's muted-panel gap). An engineering owner records that decision by commenting /approve-spec 28d31afb250d1faa47dcee8ed9f509c1a30b9e96 on this pull request, and the author may do so as an engineering owner.
These need fixing either way:
- At 2,403 authored lines this pull request carries all five #10296 commits (1,088 lines) beside its own TreeList commit, so please land #10296 first or drop those commits so this one carries only the TreeList change.
- The description still says a collapse removes the group at the tap, the rows below move at once and there is no caller burden, while this head glides the rows below, keeps a collapsing group inert and fading, and wraps every Collapsible's content in a new element, so please update it, including the #10296 changes and the evidence.
- Miss: the guide lines now sit inside the padded row wrapper with no block-start inset, so every segment starts half a row gap low and each last connector runs half a gap past its row (8 px at a 16 px
--tree-list-row-gap), whichTreeList.doc.mjssays cannot happen;insetBlockStart: 0on the guide container and a browser case at a widened gap would fix it.
The parent guide line that broke at rest in the last review now runs unbroken, and in Chromium the touch motion follows the shared glide's timing, animates only opacity and translate, and cross-fades under Reduce Motion.
[Automated review]
The still clip sat on the element's border box, so the sliding parts passed through its padding, where the content before it can draw: a Collapsible trigger's label descenders hang there. The clip now starts at the content box on the side the parts come from. The changeset names the muted Table panel, which keeps the moving clip.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on this head's spec changes: component:TreeList FR8 (the touch glide and slide inside the tree) and DD5, and the records this branch brings from #10296 (spec:AST-105 FR9's block-edge slide, architecture:react-component-runtime INV12's child walk, component:Collapsible AV5 and component:Table's muted-panel gap). An engineering owner records that decision by commenting /approve-spec 6995cb02dea3cda352eca5bfd85bbc722e1f5105 on this pull request, and the author may do so as an engineering owner.
These need fixing either way:
- At 2,535 authored lines this pull request carries all eight #10296 commits (1,171 lines) beside its own TreeList commits, so please land #10296 first or drop those commits so this one carries only the TreeList change.
- The description still says a collapse removes the group at the tap, the rows below move at once, there is no caller burden and FR8 is approved, while this head glides the rows below, keeps a collapsing group inert and fading, wraps every Collapsible's content in a new element and has no approval for this head, so please update it, including the #10296 changes and the evidence.
The guide lines now start at their row wrapper's top edge and match main at rest, including at a widened row gap. One optional addition: the touch collapse case could assert that the leaving group is inert, as FR8's verification row says it does.
[Automated review]
| const height = (await parts(page).group.boundingBox())?.height ?? Number.NaN; | ||
| const frames = await recordFrames(page, async () => tapToggle(page)); | ||
| const leaving = frames.filter(frame => frame.group != null); | ||
| // The group stays while it leaves, fading: it does not go at the tap. |
There was a problem hiding this comment.
Non-blocking (Miss): FR8's verification row (TreeList.spec.md:295) says this spec fails "a collapsed group's rows in the accessibility tree after the tap", but no case here checks it. The behavior holds: under a finger the group is inert from the collapse's commit through its exit. One assertion after the tap would make the row true, for example:
await expect(parts(page).group).toHaveAttribute('inert', '');A second change in the area, such as a nested row expanding while its parent's group still slides, read a part's running slide as the app's own move. The part then landed at once, and the rows after it jumped. A part's slide is now its glide, so the next change carries its rest into the part's new glide. A ride reads each glide on its own clock, so a nested reveal rides a neighbour that an earlier change set moving.
A ride retimes a slide, so the part's glide reads the slide's length off the slide. It records the part's ancestors' scale, read with the part's box. A part that an earlier change still glides keeps that glide, and the element keeps the moving clip.
Under a coarse pointer without a handle, the tree runs its own animated layout: the rows below glide on the shared glide while the group slides and fades on its frames, and the chevron turns on the same timing. The tree clips its bottom edge while it moves. Each row draws only its own part of the guide lines, reading each column from the ancestor whose connector sits there. TreeList FR8 takes the spec owners' wording, and the guide behavior is a proposed design decision (DD5).
Inside the padded row wrapper, the absolute guide container took its static place at the wrapper's content edge, so each segment started half a row gap low and a last connector ran half a gap past its row. It now starts at the wrapper's top edge.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on this head's spec changes: component:TreeList FR8 (the touch glide inside the tree) and DD5, and the records this branch brings from #10296 (spec:AST-105 FR9's block-edge slide, architecture:react-component-runtime INV12's child walk, component:Collapsible AV5 and component:Table's muted-panel gap). An engineering owner records that decision by commenting /approve-spec 274ff878117c4b43ab8382d5333d1e42ef0054a0 on this pull request (the author may do so as an engineering owner), and the two points below need fixing either way:
- At 2,766 authored lines this pull request carries all ten #10296 commits (1,402 lines) beside its own TreeList commits, so please land #10296 first or drop those commits so this one carries only the TreeList change.
- The description still says a collapse removes the group at the tap, the rows below move at once, there is no caller burden and FR8 is approved, while this head glides the rows below, keeps a collapsing group inert and fading, wraps every Collapsible's content in a new element and has no approval for this head, so please update it, including the #10296 changes and the evidence.
The two #10296 commits added since the last review keep a nested row opened mid-slide in step with the rows after it, and that case passes here while it fails against main. One optional addition: the touch collapse case could assert that the leaving group is inert, as FR8's verification row says it does.
[Automated review]
Stack: #10296 → #8088. Review only this change: vjeux/astryx@dbe8bcf...274ff87
User need
On iOS, opening a folder in an outline list moves the rows below it down as its children appear. Under a finger, a TreeList's children appear in one frame.
Problem and solution fit
Under a coarse pointer, an expand glides the rows below on the shared glide while the children slide in under the row and fade, and the chevron turns with them; a collapse reverses it. A mouse keeps today's tree. Without a handle, content after the tree takes its place at once (AST-105 DEC-1), and the tree clips its bottom edge while it moves, as a UITableView clips to its bounds, so rows glide in from its edge.
Current specification
component:TreeListFR8 takes the spec owners' wording, credited to the owner's feedback. DD5, the guides, stays proposed until this card passes. AV5 is main's wording again.Public delta and caller burden
None.
Design and alternatives
Evidence
The glide's 300 ms comes from #10261, now on main:
spec:AST-105FR4 and DEC-3 set--duration-medium, and FR5 applies the glide under every pointer.Before (main) and after (6995cb0; this head plays the same frames), 4× slower, under a finger:
Measured at this head's preview: the rows below, the group's slide and its fade run 300 ms, the Neutral glide, and the chevron turns in 300 ms. Before, the slide ran 125 ms.
Writing rubric results
R1 narration: FR8's "no longer use" is now present tense ("take the glide's timing, not
--duration-fast"). R2 research: none. R3 evidence: the FR8 and DD5 verification rows name the per-frame browser cases. R4 rejected alternatives: none. R5 change references: DD5 names the card, in the spec owners' wording. R6 open questions: none.Scope
Testing
What was run at 274ff87 (WebKit in the Linux CI image): TreeList's specs record every frame, with the motion specs in Chromium on a 2-core pin twice (316 passed) and TreeList's in WebKit three times (60 passed; Chromium-only cases skip). At the base they fail: the group shows at rest on its first frame with a 126 px blank band above the rows below, the collapse removes it at once, and only the chevron runs, on 125 ms. Each red-first case fails without its fix: a guide line ran 15 px past the last row it joins; a collapse painted the last row 99 px past the tree's edge; a parent's line broke at an expanded child (Themed guides); at a 16 px gap, connectors ran 8 px past their rows. Focus rings on the first and last rows stay whole while the tree moves, and WebKit meets Chromium's 1.5 px band. Also: TreeList's accessibility contracts (18), TreeList and motion units (1,056), lint (0 errors), the core type check, check:repo, check:knowledge, changeset coverage and review:pre (2,766 authored lines, #10296's included); test:ui (896 files, 21,937 tests) and test:node (936 files, 16,938 tests), with no unhandled errors.