Repository navigation
Conversation
|
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.
|
github-actions
Bot
added
the
needs:design-review
2d10275 to
47a5ca9
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is waiting on an engineering-owner decision. No current spec governs List.onReorder, ListItem.isReorderDisabled or the UIKit reorder model (rows slide aside, springs kept under Reduce Motion, no insertion line), so an owner needs to settle that contract and which record governs it: an amended List contract, or spec:AST-094 with its draft requirements approved. Once the governing record states it, any engineering owner, the author included, can record the decision by commenting /approve-spec <full head SHA>. These still need changes whatever that decision is:
- The diff against
mainis about 31,000 authored lines from the predecessor stack. Land those pull requests or update the branch so this one carries only the reorder change. - Keyboard focus on a reorder handle draws two focus rings, the handle's and the whole row's, and the handle's ring is clipped at the list's edge.
- The stacked touch row minimum heights change default List row geometry, which the List contract's FR2 says
paddedpreserves.
[Automated review]
| onPointerDown={(event: PointerEvent<HTMLButtonElement>) => | ||
| event.stopPropagation() | ||
| } | ||
| {...focusOutlineProps.focusVisible(styles.handle)}> |
There was a problem hiding this comment.
Keyboard focus here also lights the row: Item paints focusOutlineProps.focusWithin for any focused descendant, so a focused handle shows two 2 px rings (measured in Chromium), against architecture:interaction-modality INV3. The handle's own ring also loses its inline end to the list's overflow-inline: clip because of marginInlineEnd: -8. Excluding the handle from the row's focus-within match and keeping the ring inside the clip would leave one indicator.
| */ | ||
| export const listRowStyles = stylex.create({ | ||
| touchOneLine: { | ||
| minHeight: {default: null, [TOUCH]: listRowConsts.touchRowHeight}, |
There was a problem hiding this comment.
Every ListItem gets this minimum on a coarse pointer (applied in ListItem.tsx lines 456-460), so default lists grow from 41 px to 52 px rows on phones. component:List FR2 says "padded MUST preserve current header, list, and row geometry", so this needs that record amended or should stay out of this diff.
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.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/CheckboxList | +13.8KB |
+78B |
| core/CheckboxListItem | +4.3KB |
+611B |
| core/ContextMenu | +18.0KB |
+669B |
| core/List | +13.9KB |
+184B |
| core/ListItem | +4.3KB |
+611B |
| core/Markdown | +18.1KB |
+665B |
| core/PowerSearch | +12.2KB |
+355B |
| core/Table | +17.9KB |
+659B |
| core/TableCell | +17.9KB |
+661B |
| core/TableHeaderCell | +17.9KB |
+653B |
| core/TreeList | +11.5KB |
+384B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 5.0KB | 1.2KB | 1.3KB | +31B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 35.4KB | 36.6KB | +1.2KB |
Accessibility Audit
Status: 2 accessibility violation(s) found — 2 serious.
Radio group pattern - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 3/11 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
RadioList - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 3/14 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
Visual Regression
55 of 504 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
47a5ca9 to
d67ffca
Compare
…al List reorder
There was a problem hiding this comment.
Approval is waiting on an engineering-owner decision: no current spec governs List.onReorder, ListItem.isReorderDisabled or the UIKit reorder model (rows slide aside, springs kept under Reduce Motion, no insertion line), so an owner needs to settle that contract and which record governs it, an amended List contract or spec:AST-094 with its draft requirements approved. Once the governing record states it, any engineering owner, the author included, can record the decision by commenting /approve-spec <full head SHA>. These still need changes whatever that decision is:
- The diff against
mainis about 31,000 authored lines from the predecessor stack; land those pull requests or update the branch so this one carries only the reorder change. - Keyboard focus on a reorder handle draws two focus rings, the handle's and the whole row's, and the handle's ring is clipped at the list's edge.
- The stacked touch row minimum heights change default List row geometry, which the List contract's FR2 says
paddedpreserves. spec:AST-094FR7 cites an AST-090 record that does not exist in the repository.
[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.
No AST-090 record exists in the repository, at this head or on main, so an owner cannot check what FR7 departs from. Cite the record where it lives, or drop the departure until it lands.
| * 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.
ListReorder.interaction.browser.spec.ts is not in the tree at this head (the SYNC note in useListReorder.ts line 44 names it too). Drop the reference or land the browser proof with this change.
|
On the predecessor stack: this pull request is stacked, as its first line says, and only its last commit (d67ffca, 10 files, +1,703 −7) is its own. It rebases onto main as each predecessor lands, and the diff then shrinks to that commit. |
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
The reorder also now keys its motion on the resolver's touch presentation ( |
…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 are needed before this can land, although the reorder contract and the drag's look now follow the current specs everywhere except reorder mode's missing hint. Please address these:
- Update the branch once #8138 and #8336 land: 38,226 authored lines change against
main, and 32,809 of them are that open stack. - Ship reorder mode's hint (Move up, Move down and Done) here, or land #8337 with this pull request, since
spec:AST-103FR4 andspec:AST-094FR39 need a way to move a row with a mouse and no drag. scripts/touch-support-bundle.test.mjsfails “is absent from the root entry” becausecreateReorderablebrings the gesture modules into the package root, so #8671 (or another approved change to that check) has to land first.- Add mouse browser tests for the drag, drop and cancel (
spec:AST-103FR3), here or by landing #8140 with this pull request.
[Automated review]
| [controller, isTouch], | ||
| ); | ||
| return ( | ||
| <span id={controller.hintId} hidden> |
There was a problem hiding this comment.
Reorder mode renders only these hidden instructions: no hint with move buttons and Done. A mouse user who clicks a handle then has nothing on screen to move the row or end the mode (spec:AST-103 FR4, FR4a; spec:AST-094 FR39). Fold the hint in, or land #8337 with this pull request.
| @@ -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 export brings the reorder engine, and through ListReorderTouch.ts 14 gesture modules (61,003 minified bytes), into the root entry, so main's scripts/touch-support-bundle.test.mjs fails “is absent from the root entry”. #8671, or another approved change to that check, has to land first.
| * Enters reorder mode for the item with `key` from the app's own UI, such | ||
| * as a Move item in its context menu or a toolbar button, as activating | ||
| * its handle does: the focus moves to the handle, which scrolls into view, | ||
| * and the mode's keys, hint, announcements and exits follow. A key that |
There was a problem hiding this comment.
This promises a hint that this revision does not render; keep “hint” only if #8337 lands together with this pull request.
…e touch target size from the theme) under the reorder values
There was a problem hiding this comment.
Changes are needed before this can land, although the reorder contract and the drag's look follow the current specs everywhere except reorder mode's missing hint. Please address these:
- Update the branch once #8138 and #8336 land: 38,474 authored lines change against
main, and 33,057 of them are that open stack. - Ship reorder mode's hint (Move up, Move down and Done) here, or land #8337 together with this pull request, since
spec:AST-103FR4 andspec:AST-094FR39 need a way to move a row with a mouse and no drag. scripts/touch-support-bundle.test.mjsfails "is absent from the root entry" becausecreateReorderablebrings the gesture modules into the package root, so #8671 (or another approved change to that check) has to land first.- Add mouse browser tests for the drag, drop and cancel (
spec:AST-103FR3), here or by landing #8140 with this pull request.
[Automated review]
| [controller, isTouch], | ||
| ); | ||
| return ( | ||
| <span id={controller.hintId} hidden> |
There was a problem hiding this comment.
Reorder mode renders only these hidden instructions: no hint with move buttons and Done. A mouse user who clicks a handle then has nothing on screen to move the row or end the mode (spec:AST-103 FR4, FR4a; spec:AST-094 FR39). Fold the hint in, or land #8337 with this pull request.
| @@ -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 export brings the reorder engine, and through ListReorderTouch.ts 14 gesture modules (61,052 minified bytes), into the root entry, so main's scripts/touch-support-bundle.test.mjs fails "is absent from the root entry". #8671, or another approved change to that check, has to land first.
| * Enters reorder mode for the item with `key` from the app's own UI, such | ||
| * as a Move item in its context menu or a toolbar button, as activating | ||
| * its handle does: the focus moves to the handle, which scrolls into view, | ||
| * and the mode's keys, hint, announcements and exits follow. A key that |
There was a problem hiding this comment.
This promises a hint that this revision does not render; keep "hint" only if #8337 lands together with this pull request.
|
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. |
… hold A divided List row keeps its dividers while it is pressed, as it does today, and Core keeps no selection hold: products show a row kept selected through ListItem's isSelected. This removes the touch press's divider hiding (its ListItem styles, docs, changeset, tests and browser proofs) and the selection hold (selectionHold.ts, its held press state in the shared press code, its internal deselect constants, the row focus-return lines, story, changeset and proofs). The shared test helpers, the press-coverage guard and the press release's own clock stay. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
The previous merge left List.tsx's hasDividers JSDoc unresolved and the List stories empty. This restores the stories (without the selection hold demo) and the plain hasDividers line. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
There was a problem hiding this comment.
Changes are needed before this can land: the List reorder follows current spec:AST-103 in most places, but the branch carries the open predecessor stack, conflicts with main, and misses four current AST-103 requirements. Please address these:
- Update the branch once #8138, #8336, #8106 and #8076 land: 36,294 authored lines change against
main, 30,875 of them that stack, and it now conflicts inTreeList.tsx,TreeListItem.tsxandroutes.lock.json. - Ship reorder mode's hint (Move up, Move down and Done) here or land #8337 with this, since AST-103 FR4 and
spec:AST-094FR39 need a way to move a row with a mouse and no drag. - Let a pen, and a finger where the primary pointer is fine, drag a row (AST-103 FR3, AST-094 FR43), and add
attachandpositions(AST-103 FR1, FR4b), here or by landing #8683 and #8719 with this. scripts/touch-support-bundle.test.mjsfails “is absent from the root entry” becausecreateReorderablebrings the gesture modules into the package root, so #8671 (or another approved change to that check) has to land first.- Add mouse and pen browser tests for the drag, drop and cancel (AST-103 FR3), here or by landing #8140 with this.
[Automated review]
| return () => {}; | ||
| } | ||
| const onPointerDown = (event: PointerEvent) => { | ||
| if (event.pointerType !== 'mouse' || event.button !== 0) { |
There was a problem hiding this comment.
This path takes only a mouse, and the hold attaches only under a coarse primary pointer (line 1866), so on a touchscreen laptop or with a desktop pen tablet a pen or a finger cannot drag a row. Current spec:AST-103 FR3 has a pen on a fine primary pointer drag without a hold and a finger keep the hold, and spec:AST-094 FR43 attaches gestures for every pointer; #8683 carries that change, so fold it in or land it with this.
| export function createReorderable< | ||
| TKey extends string | number = string | number, | ||
| TTo = number, | ||
| >(options: ReorderableOptions<TKey, TTo>): Reorderable<TKey, TTo> { |
There was a problem hiding this comment.
Current spec:AST-103 FR4b says the object exposes attach(container, {items}), and FR1 gives the options an optional positions(); this object has only start, and the options only onReorder and isReorderable, so the spec's own attach example does not type-check. Fold in #8719, land it with this, or have an engineering owner record on main that they ship later.
| * Off by default; with a mouse nothing moves. | ||
| * @default false | ||
| */ | ||
| hasAnimatedChanges?: boolean; |
There was a problem hiding this comment.
This boolean comes from the predecessor stack (#8106). Current component:List FR12 gives this job to an animatedLayout handle from useAnimatedLayout (spec:AST-105 FR1), and AST-105 FR6 says “no other API does this job; and it is not a visual boolean”, so shipping hasAnimatedChanges adds a second public shape that a later release would have to remove.
|
An offer that brings #8106's new head into this PR: offer/list-reorder-animated-layout fast-forwards this branch with four commits.
The List, reorder and motion unit tests, typecheck, lint and If you want it here, merge the branch. |
…t and the route lock - TreeList.tsx and TreeListItem.tsx: main's meta#8568 (useAnimatedLayout and the TreeList glide) against the tree expansion change carried through meta#8106. Resolved as meta#8088's own merge of that main resolves the same conflict (6801c11): the row renders its children through renderChildren, plays its touch slide only without an animated-layout handle (useTreeExpansionMotion's isEnabled), and a group collapsing under a handle is marked as exiting and inert; this branch's revert (dividers stay drawn on press, no Core selection hold) stays applied. - internal/shadcn-registry/routes.lock.json: regenerated by the docsite's generate-data.mjs (UPDATE_SHADCN_ROUTE_LOCK=1); it lists both hook-create-reorderable and main's hook-use-animated-layout.
There was a problem hiding this comment.
Changes are needed before this can land: the merge with main resolves the earlier conflict, but the branch still carries the open predecessor stack, fails main's root-entry bundle test in CI, and misses parts of current spec:AST-103. Please address these:
- Update the branch once #8138, #8336, #8106 and #8076 land: 36,308 authored lines change against
main, about 30,889 of them that stack. - Land #8671, or another approved change to
scripts/touch-support-bundle.test.mjs, first:createReorderable's root export brings 14 gesture modules into the root entry, andtest-nodefails on it. - Ship reorder mode's hint (Move up, Move down and Done) here, or land #8337 with this, since AST-103 FR4 and
spec:AST-094FR39 need a way to move a row with a mouse and no drag. - Let a pen, and a finger where the primary pointer is fine, drag a row (AST-103 FR3, AST-094 FR43), and add
attachandpositions(FR1, FR4b), here or by landing #8683 and #8719 with this. - Show the insertion line as soon as reorder mode starts (FR4), as your offered branch does, and add mouse and pen browser tests for the drag (FR3), here or with #8140.
[Automated review]
| [controller, isTouch], | ||
| ); | ||
| return ( | ||
| <span id={controller.hintId} hidden> |
There was a problem hiding this comment.
Reorder mode renders only this hidden description: no hint with move buttons and Done, so a mouse user who clicks a handle has nothing on screen to move the row or end the mode (spec:AST-103 FR4, FR4a; spec:AST-094 FR39). Fold the hint in, or land #8337 with this pull request.
| return () => {}; | ||
| } | ||
| const onPointerDown = (event: PointerEvent) => { | ||
| if (event.pointerType !== 'mouse' || event.button !== 0) { |
There was a problem hiding this comment.
This path takes only a mouse, and the hold attaches only under a coarse primary pointer (line 1866), so a pen, or a finger on a touchscreen laptop, cannot drag a row. Current spec:AST-103 FR3 has a pen on a fine primary pointer drag without a hold and a finger keep the hold, and spec:AST-094 FR43 attaches gestures for every pointer; #8683 carries the change.
| */ | ||
| private placeModeLine(list: HTMLElement): void { | ||
| const row = this.rowOf(this.modeHandle); | ||
| if (row == null || this.lastStep == null) { |
There was a problem hiding this comment.
With lastStep null the line is cleared, so entering reorder mode shows no insertion line until the first arrow press. Current spec:AST-103 FR4 has the line mark the item's place for as long as it is in reorder mode; the fix described in comment 6467628718 would close this.
| export function createReorderable< | ||
| TKey extends string | number = string | number, | ||
| TTo = number, | ||
| >(options: ReorderableOptions<TKey, TTo>): Reorderable<TKey, TTo> { |
There was a problem hiding this comment.
Current spec:AST-103 FR4b says the object exposes attach(container, {items}), and FR1 gives the options an optional positions(); this object has only start, and the options only onReorder and isReorderable, so the spec's own attach example does not type-check. Fold in #8719, land it with this, or have an owner record on main that these ship later.
| @@ -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 export brings the reorder engine, and through ListReorderTouch.ts 14 gesture modules (61,052 bytes), into the root entry, and CI's test-node (2/4) fails scripts/touch-support-bundle.test.mjs “is absent from the root entry” on it. #8671, or another approved change to that check, has to land first.
| * Off by default; with a mouse nothing moves. | ||
| * @default false | ||
| */ | ||
| hasAnimatedChanges?: boolean; |
There was a problem hiding this comment.
This boolean comes from the predecessor stack (#8106). Current component:List FR12 gives this job to an animatedLayout handle from useAnimatedLayout (spec:AST-105 FR1), and AST-105 FR6 says “no other API does this job; and it is not a visual boolean”, so shipping hasAnimatedChanges adds a public shape that would need a breaking removal.
Depends on #8138 and #8336. Review only this PR's change: vjeux/astryx@fba78a3...9e84c6c
Why
People reorder playlists, to-do lists and settings on their phones.
Listcan't do it, so apps build their own, often without a keyboard or screen-reader path.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.
spec:AST-103defines a reorderable List, and #8138 holds the measured UIKit values it plays. Here,createReorderable({onReorder})makes the behavior,Listtakes it asreorderable, and eachListItemwith anitemKeygets a handle:onReorder({key, to})once.Options and tradeoffs
Why this choice
design:ordered-collection-reorderingchose still rows and a line, because they need no layout work during the drag and look the same everywhere. The hold is UIKit's measured 150 ms. Only the handle is added (FR5), so rows keep their layout. List grows by 146 B gzip and ListItem by 546 B; CSS doesn't change.How to check it
In Storybook
Core/List, on a phone or with touch emulation:I ran the unit tests and the touch and mouse browser tests in Chromium and WebKit, and all pass. Two reds come from elsewhere:
touch-support-bundle, which #8671 fixes, andpr-rtl, main's newer RTL test from 0472c4e.Evidence
On iOS 26.2 Safari: a held drag, and the drop with the main thread blocked.
Stack (merge in this order): the list stack through #8106 → #8138 and #8336 → this → #8337 → the browser proofs.