Repository navigation
Conversation
…matedLayout handle component:List FR12: with the handle in List's context, a row reports a change to its start or end content. Content that appears enters, content that leaves exits (inert and hidden from assistive technology, its focus handed to the row's target without scrolling), content replaced in the same slot swaps in place (spec:AST-105 FR4, FR8), and the row's other content and the rows after it glide. A same-size replacement and a re-render of the same control do not animate; a leaving slot that sets the row's height hides at once (holdLayoutExit). Item marks its slot wrappers so the row can find them.
…odes story A leaving or replaced slot reports itself, so its exit is a measured one and a leaving slot that sets the row's height goes at once (holdLayoutExit drops its change); content that appears reports from the row's label. A swap's copy and a leaving wrapper are never taken for the live slot. Animated Edit Modes composes select (a CheckboxInput in startContent with isSelected) and delete (a remove IconButton in endContent that swaps for a Delete Button) inline in its own render. ListItemSlots.a11y.browser.spec.ts proves, at 1280 and 390 px: a Checkbox enters while the label glides; a replaced control swaps in place and hands its focus to the row without scrolling; a leaving control is inert and hidden as its exit starts; a same-size toggle plays nothing; a height-setting slot goes at once; Reduce Motion with a mouse changes at once. The four motion cases fail with the reporting turned off. List's doc entry and a Changeset describe it.
|
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.
|
PR Analysis Report
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the slot motion matches component:List FR12 in the Animated Edit Modes story, but two defects need fixing before this can land.
identityOfstores element types in aWeakMap, which throwsInvalid value used as weak map keyfor a Fragment or Suspense, so anyListItemwith fragmentstartContentorendContentnow fails to render, with or withoutanimatedLayout(thedashboard-progresstemplate's risk rows pass one).- In the story, Edit → Done hides three rows' end slots at once but lets the last row's slot play its exit while the rows above it glide; FR12 has a slot that sets the row's height hide at once, and
holdLayoutExitskips that check when nothing follows the slot.
Please add a test for each: a fragment slot, and an end slot that sets the last row's height.
[Automated review]
| const known = typeIds.get(type as object); | ||
| const assigned = known ?? nextTypeId++; | ||
| if (known == null) { | ||
| typeIds.set(type as object, assigned); |
There was a problem hiding this comment.
typeIds.set throws TypeError: Invalid value used as weak map key when type is a registered symbol (Fragment, Suspense, StrictMode, Profiler), and identityOf runs on every ListItem render, so startContent={<>…</>} now fails to render with or without animatedLayout; the dashboard-progress page template's risk rows pass a fragment endContent. Keying only objects and functions here, and naming a symbol type by String(type), keeps the identity without the throw.
| change.animate(); | ||
| } | ||
| for (const element of leaving) { | ||
| if (!holdLayoutExit(animatedLayout, element, () => element.remove())) { |
There was a problem hiding this comment.
This relies on holdLayoutExit refusing a slot that sets the row's height, but giveUpSpace returns true without that check when nothing follows the element in the area (motion/animatedLayout.ts:1246-1251). An end slot in the last row takes that path: in the Animated Edit Modes story, Edit → Done hides three rows' end slots at once while Archive's plays its exit at 48 px and the rows above it glide, which FR12 rules out for a height-setting slot.
| }) => { | ||
| await mount(page); | ||
| await page.addStyleTag({ | ||
| content: '[data-astryx-item-slot="start"] { min-height: 96px; }', |
There was a problem hiding this comment.
This exercises a start slot, which always has the label after it, so it never reaches the path where nothing follows the slot. An end slot that sets the last row's height (the story's own Edit → Done) plays an exit today; a case for it would catch that.
…eight goes at once CodeBunny on #8803: - identityOf keyed every non-string element type in a WeakMap, which throws for a symbol type (Fragment, Suspense), so a ListItem with a fragment slot failed to render, with or without a handle. A symbol type is named by String(type). Unit case; it fails on the previous head. - holdLayoutExit's giveUpSpace returned early when nothing follows the element in the area, before the check that a flex-row item setting its row's height frees no block space, so the last row's end slot played its exit while the rows above it glided (spec:AST-105 Coverage, component:List FR12). With nothing after it, a flex-row item that sets its row's height is refused too. Browser case (the story's Edit then Done); it fails on the previous head. The leaving-control case sets rows taller than their controls, so its slots exit.
There was a problem hiding this comment.
Changes requested: the latest commit fixes the fragment crash and the last row's end slot, but one path of the same height rule remains. In rows with align="start", a start slot that sets the row's height in the last row still plays its exit on Select → Done while the rows above it glide, where component:List FR12 has it hide at once. Running the isRow && setsRowHeight(element) check in giveUpSpace for every flex-row item, not only when nothing follows the element, fixes it; please add that case to ListItemSlots.a11y.browser.spec.ts.
[Automated review]
| // Nothing after it in the area to land anywhere, but a flex-row item | ||
| // that sets its row's height still frees no block space with an inline | ||
| // margin: the row would keep its height through the exit, then jump. | ||
| if (isRow && setsRowHeight(element)) { |
There was a problem hiding this comment.
This check only runs when nothing follows the element. A start slot always has the row's content after it, so it takes the margin path below, which refuses a height-setting slot only if what follows the row or that content moves once the slot is out of flow. In the last row of align="start" rows neither does: in the Animated Edit Modes story with align="start", Select → Done plays Archive's checkbox exit with the row held at 40 px while the rows above it glide, where component:List FR12 has it hide at once. Moving if (isRow && setsRowHeight(element)) return false; above nextInFlow makes that case hide at once with nothing playing, and Select and Remove → Delete still animate.
| * so a gap or collapsing margins come out exact. Returns false, setting | ||
| * nothing, when no margin reaches that place. | ||
| */ | ||
| /** |
There was a problem hiding this comment.
Nit: setsRowHeight landed between giveUpSpace's doc comment (lines 1226–1232) and giveUpSpace, so that comment now sits on top of this one and giveUpSpace has none. Moving setsRowHeight above line 1226 keeps each comment on its function.
…herever it sits CodeBunny on #8803: the height check ran only when nothing follows the element, so a start slot (always followed by the row's content) setting the last row's height in align="start" rows took the margin path, whose landing check saw nothing move, and played its exit while the rows above it glided (component:List FR12, spec:AST-105 Coverage). giveUpSpace now refuses a flex-row item that sets its row's height before anything else. setsRowHeight sits above giveUpSpace's doc comment. Animated Edit Modes gains an align control; the height-setting start slot case runs at align center and start, and the start one fails on the previous head.
There was a problem hiding this comment.
Changes requested: entering checkboxes, the Remove → Delete swap and the focus handoff work as component:List FR12 describes, and the latest commit fixes the align="start" last-row exit, but a leaving end-slot control does not exit in place. In rows taller than their trailing control, the exit's negative margin-inline-end moves the end slot (which has margin-inline-start: auto) its own width past the row's edge, where the list's inline clip hides it at once, so nothing visibly exits. Please keep the leaving end slot's box where it was, for example by exiting it as an inert copy over its old box as the swap does, and have ListItemSlots.a11y.browser.spec.ts assert that the leaving slot keeps its position. Two smaller points: a control that keeps its type and key but changes size is never reported, though FR12 exempts only same-size changes, and the description shows head frames only, where spec:AST-105 FR5 asks for before and after.
[Automated review]
| change.animate(); | ||
| } | ||
| for (const element of leaving) { | ||
| if (!holdLayoutExit(animatedLayout, element, () => element.remove())) { |
There was a problem hiding this comment.
An end slot that leaves without setting its row's height goes through giveUpSpace's negative margin-inline-end. Item's end slot has marginInlineStart: 'auto', so that moves the slot its own width toward the row's end, and the list's overflowInline: 'clip' hides it in the first frame. In the Animated Edit Modes story with 72 px rows, Edit → Done moves each remove button from 1224–1256 px to 1256–1288 px, past the row's edge at 1264 px, so nothing visibly exits (FR12: "content that leaves exits"). Keeping the leaving end slot's box where it was fixes it, for example by exiting it as an inert copy laid over its old box, as the swap does.
| // The leaving control conceals in place (an exit, not a removal). | ||
| expect( | ||
| played.some( | ||
| entry => entry.target === 'copy' && entry.first.clipPath != null, |
There was a problem hiding this comment.
This passes while the leaving remove buttons sit 32 px past the row's edge, clipped out of view. Asserting that each leaving slot keeps its left edge (its getBoundingClientRect().left before and after Done) would catch it.
| name: 'animatedLayout', | ||
| type: 'AnimatedLayout', | ||
| description: 'The handle from useAnimatedLayout({containerRef}). With it, when rows are inserted, removed or reordered, an inserted row enters, a removed row collapses in place, a row that crosses rows moving the other way rides above them with the surface color behind it, and the rows after a change and the content after the list, up to that container, glide to their new places, all on one shared motion, under every pointer. Under Reduce Motion nothing travels. A row is identified by its itemKey: a row without one changes at once. Without a handle the list changes at once.', | ||
| description: 'The handle from useAnimatedLayout({containerRef}). With it, when rows are inserted, removed or reordered, an inserted row enters, a removed row collapses in place, a row that crosses rows moving the other way rides above them with the surface color behind it, and the rows after a change and the content after the list, up to that container, glide to their new places, all on one shared motion, under every pointer. Under Reduce Motion nothing travels. A row is identified by its itemKey: a row without one changes at once. With the handle, changes to the startContent and endContent of a row animate too: a control that appears enters, one that leaves exits (inert, its focus handed to the row without scrolling), one replaced in the same slot swaps in place, and the label of the row glides along it; a replacement at the same size, or the same control re-rendering, does not animate. Without a handle the list changes at once.', |
There was a problem hiding this comment.
FR12 exempts only "a slot whose content changes without changing size". This text exempts "the same control re-rendering" at any size, which matches the type-and-key identity in listItemSlotMotion.tsx but not FR12. Either report a same-identity slot whose size changed, or limit the exemption to a re-render at the same size.
…ges size in place is reported CodeBunny on #8803: - A leaving end slot exited by a negative inline-end margin, which moved the slot (margin-inline-start: auto) its own width past the row's edge, where the list's inline clip hid it at once. A leaving slot now plays out as an inert copy over its old box, as a swap's old content does, while the row's other content glides into its space; a slot that sets the row's height still goes at once (the row measures that itself, so animatedLayout.ts is unchanged here). The browser case asserts each leaving slot keeps its left edge; it fails on the previous head. - FR12 exempts only a change at the same size, but a control keeping its type and key was never reported. The row now measures a slot whenever what it shows changes, down to its non-function props; a change at the same size moves nothing, and one that resizes the slot moves the label (unit case). The doc entry and Changeset say so.
There was a problem hiding this comment.
Changes requested: the slot motion now follows component:List FR12 for entering, leaving, swapped and resized content, and the earlier end-slot jump is fixed, but two defects remain.
- A focused slot control that stays loses focus to the row: in the Animated Edit Modes story, Space on a row's Checkbox moves focus to the row's button, so a second Space activates the row instead of unchecking the box (
listItemSlotMotion.tsx:349), while FR12 moves focus only for a control that leaves. - A height-setting slot written
condition && elementplays its exit below its shrunken row, becauseidentityOfcountsfalseas content and classes the leave as a swap, skipping the height rule (listItemSlotMotion.tsx:80).
Please fix both and add a browser case for each.
[Automated review]
| }); | ||
| } | ||
| } | ||
| if (hadFocus) { |
There was a problem hiding this comment.
This runs for every changed slot that held focus, a resize included, so a control that stays loses focus to the row: in the Animated Edit Modes story, Space on "Select Drafts" (or a click) leaves focus on Drafts' row button, and a second Space activates the row instead of unchecking. FR12 moves focus only for a slot control that leaves; handing off only when the focused element is no longer connected after the commit (or only for exit and swap) keeps the leave case working.
| if (node == null) { | ||
| return null; | ||
| } | ||
| if (typeof node === 'boolean') { |
There was a problem hiding this comment.
false counts as content here, so a slot written condition && element that leaves is classed as a swap and skips the height rule at line 276. With the story's markup written mode === 'delete' && control, Edit → Done plays four inert copies that hang below their shrunken rows, where FR12 has a height-setting slot hide at once. Returning null for booleans (Item's own lint rule asks for isRenderable on the same guards) keeps the exit path and its height check.
| * the row's target without scrolling), which swaps in place, and when the | ||
| * row changes at once | ||
| * @position Unit tests for List/listItemSlotMotion.tsx; the motion itself is | ||
| * proven in List/__tests__/ListItemSlots.interaction.browser.spec.ts |
There was a problem hiding this comment.
Nit: the browser proof lives in List/__tests__/ListItemSlots.a11y.browser.spec.ts; ListItemSlots.interaction.browser.spec.ts does not exist.
…tion && control leaves as an exit CodeBunny on #8803: - Focus moved to the row for every changed slot that held it, so Space on a row's Checkbox left focus on the row and a second Space activated the row. Only a control that left (no longer in the document after the commit) hands its focus on now. Browser case (Space twice on a Checkbox); it fails on the previous head. - A boolean slot (`condition && control` leaving as false) counted as content, so the leave was classed as a swap and skipped the height rule; a height-setting slot played inert copies below its shrunken row. A boolean shows nothing, as null does. The story writes its end slot as `mode === 'delete' && …`, so the last-row height case covers it; it fails on the previous head. The unit test's header names the browser spec it pairs with.
There was a problem hiding this comment.
Changes requested: the slot motion follows component:List FR12, and the focus handoff and condition && control fixes work, but a slot that sets its row's height and leaves as '' still plays its exit below the shrunken row. identityOf returns text: for '' (packages/core/src/List/listItemSlotMotion.tsx:81-82), so the leave is classed as a swap and skips the height rule at line 277. Returning null for every value that renders nothing, as isRenderable does, and adding an '' case to ListItemSlots.a11y.browser.spec.ts would fix it.
[Automated review]
| if (node == null || typeof node === 'boolean') { | ||
| return null; | ||
| } | ||
| if (typeof node === 'string' || typeof node === 'number') { | ||
| return `text:${node}`; |
There was a problem hiding this comment.
'' falls through to text: here, so a control that leaves as '' (a text && control slot whose string is empty) is classed as a swap and skips the height rule at line 277. With the Animated Edit Modes story's end slot falling back to '' instead of false, Edit → Done plays four remove-button copies below their shrunken rows, the last past the list's end, where component:List FR12 has a height-setting slot hide at once. if (!isRenderable(node)) return null; (from ../utils/isRenderable) covers null, undefined, booleans and '' together.
CodeBunny on #8803: identityOf counted '' as text, so a control leaving as '' (a text && control slot whose text is empty) was classed as a swap and skipped the height rule. A slot shows nothing for every value isRenderable rejects: null, undefined, a boolean and ''. Unit case (a remove button leaving as '' plays out as an inert copy); it fails on the previous head. The previous push took #8106's head (e72bb2f: one update reads only the rows near the visible part, and a leaving row exits as itself); the List, Item and motion suites and the slot motion's real-Chromium spec pass on it.
There was a problem hiding this comment.
Recommending approval: the start- and end-slot motion follows component:List FR12, and a height-setting slot that leaves as '' now hides at once like one that leaves as false. The new jsdom case protects that path, and the existing browser cases cover entering, leaving, swapping, same-size changes and the focus handoff. No further changes are needed here; retarget to main once #8106 lands, as the description plans.
[Automated review]
github-actions
Bot
added
the
needs:design-review
Stacked on #8106 (List's
animatedLayouthandle in its context,component:ListFR12). Review only this PR's change: stack/list-updates-8106...feat/list-slot-motion. Retarget tomainonce #8106 lands.User need
Apps build edit modes into list rows through
ListItem's slots: a Checkbox appears instartContentto select, and a remove button inendContentturns into Delete. Today those controls pop in and out, and the label jumps sideways in one frame.Problem and solution fit
With the list's
animatedLayouthandle (read from List's context; no new props), a row reports a change to its start or end content (component:ListFR12):spec:AST-105FR8): the old control is a transient inert copy fading out while the new one fades in. If the row's height changes, the rows after it glide too.The reorder handle's appearance comes in a separate small PR stacked on #8139 (
createReorderable), once that is in reach.Size: one internal hook (
List/listItemSlotMotion.tsx). Item marks its two slot wrappers (data-astryx-item-slot) so the row can find them.Current specification
component:ListFR12, the slot clause (#8779, merged): "With a handle, a row also reports a change to its start or end content …".spec:AST-105FR4 and FR8.Public delta and caller burden
None: no new props. Rows animate their slots only inside a List given
animatedLayout.Evidence
Real Chromium, built story Core/List › Animated Edit Modes. Its own render composes select (a
CheckboxInputinstartContentwithisSelected) and delete (a removeIconButtonthat swaps for a DeleteButtoninendContent) inline, with no wrapper components."Before" is the same story with the row's reporting turned off (the list's behaviour without this PR).
ListItemSlots.a11y.browser.spec.ts: 10/10 pass at 1280 and 390 px, including a Checkbox toggled twice from the keyboard keeping its focus. These cases fail without the change: enter, swap with focus handoff, and the leaving control (which must keep its left edge). The two height-setting cases (end slot in the last row, writtencondition && control; start slot atalign="start") and the keyboard focus case fail on earlier heads of this PR.listItemSlotMotion.test.tsx(jsdom): 8 cases, including a fragment slot (which crashed an earlier head) and a control that changes size in place. The motion cases failed before the wiring.Scope
Testing
pnpm -F @astryxdesign/core typecheckand eslint.Changeset:
[feat]patch for@astryxdesign/core.