Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:design-review
e4d7ed7 to
2c3fd8d
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an owner decision. TransferList's move to core List's reorder model (owner decisions 1–4 in the description) and the draft AST-094 have no approved spec, so an engineering owner needs to provide or identify the governing records and record the decision by commenting /approve-spec <full head SHA> with this pull request's current head. The author may do this if they are an engineering owner. Whatever the decision, these changes are needed:
- Land #8139 and #8126 first, or rebase once they merge, so this pull request carries only its 1,023-line TransferList commit instead of 32,385 lines.
- In
reorderSelected, keep values that have no option in their own slots: today Space, Home, Escape on Owner with['name', 'retired', 'owner', 'status']reports['name', 'owner', 'retired', 'status']. - With
renderOption, name each reorder handle after its option rather than "Reorder item N". - The description says the keys keep their behavior, but arrow keys now move a row without a pick-up, so please include that in the owner decision.
[Automated review]
| // Next to the row it lands on, in the full value (which may hold | ||
| // values without an option). | ||
| const nextValue = currentValueRef.current.filter(item => item !== moved); | ||
| const at = nextValue.indexOf(anchor); | ||
| nextValue.splice(toIndex > fromIndex ? at + 1 : at, 0, moved); | ||
| commit(nextValue); |
There was a problem hiding this comment.
Escape replays the inverse move through onReorder(index, lifted.origin), but re-inserting the moved value next to its anchor lets entries with no option drift: with ['name', 'retired', 'owner', 'status'], Space, Home, Escape on Owner ends at ['name', 'owner', 'retired', 'status'] while List announces "back to position 2 of 3", and ArrowDown then ArrowUp on Name ends at ['retired', 'name', 'owner', 'status']. Writing the moved visible order back into the visible values' own indices keeps hidden entries in their slots and makes every move reversible.
| return ( | ||
| <ListItem | ||
| key={option.value} | ||
| label={renderOption?.(option) ?? option.label} |
There was a problem hiding this comment.
With renderOption this label is a node, so ListReorderHandle names every handle "Reorder item N" by position (ListReorderHandle.tsx:197-211), while the row's other action still reads "Remove Name"; the old grip was always "Reorder {option.label}". Giving the handle the option's text, and asserting Reorder Name in the renderOption test, would keep the handles distinguishable for screen-reader and voice-control users.
| useIsomorphicLayoutEffect(() => { | ||
| const element = listRef.current; | ||
| if (element == null) { | ||
| return; | ||
| } | ||
| const rows = rowsOf(element); | ||
| const places = new WeakMap<HTMLElement, number>(); | ||
| for (const row of rows) { | ||
| // Inside the list: content above it changing size moves no row. | ||
| const base = row.offsetParent === element ? 0 : element.offsetTop; | ||
| places.set(row, row.offsetTop - base); | ||
| } | ||
| const before = placesRef.current; | ||
| placesRef.current = places; | ||
| const height = element.offsetHeight; | ||
| const grew = height - heightRef.current; | ||
| heightRef.current = height; | ||
| // The first commit, motion off, or a reorder session that owns the rows: | ||
| // remember where rows are, move nothing. | ||
| if ( | ||
| before == null || |
There was a problem hiding this comment.
This layout Effect reads every row's offsetTop and the list's offsetHeight after every commit before it checks animates, so every List, including those without hasAnimatedChanges, forces a layout on each commit. Returning before the reads when enabled is false (and clearing the remembered places) limits the cost to lists that opt in.
2c3fd8d to
d0c7163
Compare
There was a problem hiding this comment.
Approval waits on an owner decision: TransferList's move to core List's reorder model (owner decisions 1–4 in the description, plus arrow keys that now move a row without a Space pick-up) and the draft AST-094 have no approved spec. An engineering owner needs to provide or identify the governing records and record the decision by commenting /approve-spec <full head SHA> with this pull request's current head; the author may do so if they are an engineering owner. Whatever the decision, these changes are needed:
- Land #8139 and #8126 first, or rebase once they merge, so this pull request carries only its 1,023-line TransferList commit instead of 32,445 lines.
- In
reorderSelected, keep values that have no option in their own slots: Space, Home, Escape on Owner with['name', 'retired', 'owner', 'status']reports['name', 'owner', 'retired', 'status']. - Keep the selected rows' Remove buttons in one column and centred: beside the 44 px handle, movable rows grow to 44 px and Remove moves 36 px inward and 4 px up (visible in the Locked story).
- With
renderOption, name each reorder handle after its option rather than "Reorder item N".
[Automated review]
| <ListItem | ||
| key={option.value} | ||
| label={renderOption?.(option) ?? option.label} | ||
| endContent={optionActions(option, side, index)} |
There was a problem hiding this comment.
The List handle that follows this end content is at least 44 × 44 (ListReorderHandle.tsx:56-64), so in this compact list a movable row grows to 44 px while fixed rows and every available row stay 36 px, and its Remove button sits 36 px further from the edge and 4 px above the row's centre. In the Locked story the Remove column zigzags between Owner/Last updated and Name/Status. Reserving the handle's slot on rows without one, and letting the handle fit a compact row instead of growing it, would keep one Remove column and one row height.
PR Analysis Report
View Storybook for this PR · View Sandbox for this PR Generated by PR Enrichment workflow | View current CI run |
d0c7163 to
03b352e
Compare
There was a problem hiding this comment.
Approval waits on an owner decision: TransferList's move to core List's reorder model (owner decisions 1–4 in the description, plus arrow keys that now move a row without a Space pick-up) and the draft AST-094 have no approved spec, so an engineering owner needs to provide or identify the governing records and record the decision by commenting /approve-spec <full head SHA> on this pull request, which the author may do if they are an engineering owner. Whatever the decision, these changes are needed:
- Land #8139 first, or rebase once it merges, so this pull request carries only its 1,128-line TransferList commit instead of 32,359 lines.
- In
reorderSelected, keep values that have no option in their own slots: Space, Home, Escape on Owner with['name', 'retired', 'owner', 'status']reports['name', 'owner', 'retired', 'status']. - Keep the selected rows' Remove buttons in one column beside the 44 px handle, and name each handle after its option under
renderOptioninstead of "Reorder item N". - Add the List drag browser spec that
ListReorder.test.tsxanduseListReorder.tscite, or correct those references.
[Automated review]
| * nothing, and a disabled row's handle stays focusable but inert | ||
| * @position Tests for List/useListReorder.ts and List/ListReorderHandle.tsx; | ||
| * the touch and mouse drags are proven in a real browser by | ||
| * ListReorder.interaction.browser.spec.ts |
There was a problem hiding this comment.
This names ListReorder.interaction.browser.spec.ts as the proof of the touch and mouse drags, but no such spec exists in this pull request (useListReorder.ts:43-44 and TransferList.test.tsx:6 point to it too). The mouse drag that replaced TransferList's pointer tests has no test; adding the spec, or correcting the three references, would keep the coverage claim true.
|
On the predecessor stack: this pull request is stacked, as its first line says, and only its last commit (03b352e, 4 files, +156 −972) is its own. It rebases onto main as each predecessor lands, and the diff then shrinks to that commit. |
03b352e to
1a2c33c
Compare
1a2c33c to
60cfda0
Compare
with main 6aea9bd) under the List reorder
…ntries and internal/motion Main's resolveSnapPoint test (meta#8663) keeps the release policy internal: not on the package root, not in a barrel, and no package export naming motion. The last check also caught ./internal/motion, Core's internal entry for Astryx's own packages (architecture:react-component-runtime INV13), which re-exports no release policy. The export check now covers the public keys only, and a new assertion keeps the claim as strong: internal/motion must not re-export resolveSnapPoint either. On this branch with main the test failed on ./internal/motion; it now passes, and it fails when internal/motion re-exports resolveSnapPoint.
…hat the model does astracat's 10:52Z review: the changeset still listed the slides and the drop's settle, which reorder.ts no longer holds, and ReorderRow.isFixed's doc said other rows cannot cross a fixed row, which movableRange no longer does for one in the middle. The changeset names the claim and the edge autoscroll steps, and the model's drop position, movable range and autoscroll step; isFixed says a run of fixed rows at either end bounds a drag and one in the middle is passed over (spec:AST-103 FR1).
… a mouse press At the document, an event from inside an open shadow root is retargeted to the shadow host, and so is a touch's target, so the host looked for participants from the shadow host and found none inside the root: the browser then sent its own mousedown and click. Both cases, a finger and a mouse, attach a tap to an element in the shadow root. Fail until the host resolves targets through the composed path.
…composed path Inside an open shadow root the document's listeners see the shadow host, in event.target and in a touch's target alike, so a recognizer on an element in the root never got its finger, and nothing claimed it. landedOn takes the composed path's first element for the event's own target and for a touch whose target is the event's (another finger keeps its own): a touch's start, a pointer's press and its pairing, the last touch a scroll reads, a cancelled pointer's touch and hover.
…ouch target never drops below 44 px - A click in a link row's own box, outside its anchor (its padding, or a tap in its touch hit area under a finger), goes to the row's anchor with its modifier keys, so link rows gain the reach other rows have. Item marks the row's anchor `data-astryx-row-link`; a row with both `onClick` and `href` keeps `onClick`. - The touch target is 44 px (`spec:AST-094` FR42), or more where the page sets `--size-touch-target` larger; a smaller value never lowers it. The docs and the changeset no longer tell themes to set it. - The code cites FR42, and `component:List` FR10 where it applies. - Under a finger an interactive row's `::after` is its touch hit layer, so the row press layer's notes say to decorate a row with a child. Red first: a tap in a link row's growth follows its link (browser); a click in a link row's padding reaches its anchor, modifiers included, and a row with both `onClick` and `href` keeps `onClick` (unit); a page's 30 px touch target leaves 44 px (unit, browser).
… hit area; the target never drops below 44 px) into the tree expansion change
… hit area; the target never drops below 44 px) into the list updates change
…in when the pointer or theme changes `--size-touch-target` was read once, when a row mounted, and only in px: a theme's `3.75rem` kept the 44 px reach, and a pointer that turned coarse, or a theme that changed, left the size the row first read. The size is now resolved in px, rem (through the root's font size) or em (through the element's), still never below 44 px, and read again when the primary pointer changes or the theme does. Red first (unit): a 3.75rem size and a 3em size at 20 px give 60 px; a pointer turning coarse re-reads it; a theme change re-reads it.
…again on a pointer or theme change) into the tree expansion change
…again on a pointer or theme change) into the list updates change
There was a problem hiding this comment.
Changes requested: TransferList's move onto createReorderable follows spec:AST-103 and the current reordering design, and the issues raised earlier in TransferList's own files are fixed. Two things remain, both from the stacked predecessors this branch carries:
- Land #8337, #8562 and the pull requests under them first, or rebase onto
mainas each one merges, so this pull request carries only TransferList's 2,122 changed lines instead of 41,791. scripts/touch-support-bundle.test.mjs("is absent from the root entry") fails on this head and passes onmain, because the rootcreateReorderableexport now brings the gesture host into Core's root entry; please resolve it in the predecessors before this lands.
[Automated review]
| @@ -172,6 +172,9 @@ export * from './Outline'; | |||
| // Overflow list | |||
| export * from './OverflowList'; | |||
|
|
|||
| // Reorder (a collection's `reorderable` prop) | |||
| export * from './reorderable'; | |||
There was a problem hiding this comment.
With this export, Core's root entry carries the reorder engine and with it 14 gesture modules (GestureHost.ts, GestureArena.ts and more), so scripts/touch-support-bundle.test.mjs › "is absent from the root entry" fails at this head while it passes on main. Built alone, @astryxdesign/core/reorderable carries 15 gesture modules; TransferList's and List's entries carry none. Keeping the engine out of the root entry, or changing the test with an owner's agreement, would let the node tests pass.
…inst a fine pointer astracat's 13:21Z review: the finger-target case capped every hint button, Done included, below 44 px, which neither shows that no control grows for a finger (spec:AST-094 FR42) nor allows a button that is larger by itself. It now opens the same hint where the primary pointer is fine and requires each button to add the same space around what it shows (its glyph, or its text's width and line) under both pointers; the theme's type scale may set larger text for a coarse pointer, which the comparison leaves out. The invisible hit layer still only grows, up to 44 px or the button's own size. Failing-first: with a coarse-only 2 px of block padding on the buttons, the old cap passed and this case fails (Move up adds 4 px of height).
…e touch target size from the theme) under the reorder values
There was a problem hiding this comment.
Changes requested: TransferList's move onto createReorderable follows spec:AST-103 and the current reordering design, and no defect turned up in TransferList's own files. Two things remain, both from the stacked predecessors this branch carries:
- Land #8337, #8562 and the pull requests under them first, or rebase onto
mainas each one merges, so this pull request carries only TransferList's 2,122 changed lines instead of 42,120. scripts/touch-support-bundle.test.mjs("is absent from the root entry") fails on this head and passes onmain, because the rootcreateReorderableexport brings the gesture host into Core's root entry; please resolve it in #8139 before this lands.
[Automated review]
| @@ -172,6 +172,9 @@ export * from './Outline'; | |||
| // Overflow list | |||
| export * from './OverflowList'; | |||
|
|
|||
| // Reorder (a collection's `reorderable` prop) | |||
| export * from './reorderable'; | |||
There was a problem hiding this comment.
This root export brings createReorderable's engine, and with it 14 gesture modules (GestureHost.ts, GestureArena.ts, LongPressGestureRecognizer.ts among them), into Core's root entry, so scripts/touch-support-bundle.test.mjs › "is absent from the root entry" fails on this head and passes on main. Please settle it in #8139, by keeping the engine out of the root entry or by changing the check with an owner's agreement; the description points to the open #8671 for that.
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
Depends on #8337, and carries #8562 (Core's internal entries); if those land differently, this follows her decision. Review only this PR's change: vjeux/astryx@682470b...a6bcd6d
Why
TransferList lets people order the items they picked, such as columns to show. Its grip grabbed every touch at once, so swiping over grips on a phone didn't scroll the panel.
What led to it
This is part of the UIKit-parity program, which makes Astryx's components behave like their iOS counterparts on touch screens. Moving Lab's reorders onto core's is part of it. TransferList had its own drag code, duplicating core
List's reorder (#8139). The selected panel becomes a coreListwithreorderable: hold to drag, flick to scroll, pinch to zoom, and keyboard reorder mode. Following Cindy's direction (her commits are here),reorderabletakes thecreateReorderableobject, withisReorderable(value)andonReorder({key, to}). TransferListSelector takes it too: in staged mode a move edits the draft, and Cancel drops it.Before:
<TransferList isReorderable />. After:<TransferList reorderable={createReorderable({onReorder})} />.Options and tradeoffs
Why this choice
Reusing core's reorder means one implementation, and TransferList gets the hold, the keyboard path and the announcements. The cost is the API change; Lab is private, so there's no changeset. An option that can't move shows no grip (
spec:AST-103FR1). Reordering pauses while a search filters the panel, where a position is ambiguous. Cindy'sinternal/reorder.tstakes string or number keys to match her #8592, named here for her review. TransferList shrinks by 1,131 B gzip.How to check it
In Storybook
Lab/TransferList:I ran the unit tests and the Chromium browser tests, covering moves in the owner's terms, the staged draft, and a preview that stops at the panel's end. All pass. Two reds come from elsewhere:
touch-support-bundle, which #8671 fixes, andpr-rtl, main's newer RTL test from 0472c4e.Evidence
Before: the grip drags at once, and on iOS 26.2: after, hold, drag and drop, and a pinch zooms.
Stack (merge in this order): … → #8138 → #8139 → #8337 → this.