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
PR Analysis Report
View Storybook for this PR · View Sandbox for this PR 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 waits on an engineering owner's decision on direction that no current spec covers: draft spec:AST-094 FR1–FR23 with DEC-1 and DEC-2, and the four ListInput decisions the description marks as pending owner review. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Whatever that decision is, these still need changes:
- Land or retarget the stack so this diff is only the ListInput change; it now carries 34,464 authored lines, most of them from eight other pull requests.
- Add the real-browser test that
useListReorder.tsandListReorder.test.tsxpoint to (ListReorder.interaction.browser.spec.ts), covering the slide, autoscroll, drop and pointer cancel. - Remove the document touch listeners
ListReorderHandleinstalls once no handle needs them, and fix AST-094's references to aspec:AST-090that does not exist.
[Automated review]
| y: recognizer.location().y, | ||
| }); | ||
| }; | ||
| useEffect(() => { |
There was a problem hiding this comment.
This effect installs capture touchstart, touchend and touchcancel listeners on the document through countFingers (lines 106–118) and returns no cleanup, so they stay for the document's lifetime and one module-level fingersDown serves every document. architecture:react-component-runtime INV3 requires cleanup to release what setup installs; counting handles per document and removing the listeners with the last one would meet it.
| * row springs back to its place. | ||
| * | ||
| * SYNC: When modified, update List/List.tsx, List/ListReorderHandle.tsx and | ||
| * List/__tests__/ListReorder.interaction.browser.spec.ts. |
There was a problem hiding this comment.
List/__tests__/ListReorder.interaction.browser.spec.ts is not in this head, so the drag, slide, autoscroll, drop and pointer-cancel paths this comment hands to it have no real-browser test, which spec:AST-013 FR10 asks for. Add the spec, or point this comment (and the headers of ListReorder.test.tsx and ListInput.test.tsx) at evidence that exists.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
There is no spec:AST-090 on main or at this head, so this proposed amendment, and the one in Current-state impact, has no record for its owners to rule on. Cite the record that states these exit rules, or drop the amendment.
| releaseMotion(child, 'translate'); | ||
| } | ||
| element.style.removeProperty('overflow'); | ||
| setShown(false); |
There was a problem hiding this comment.
This layout effect sets shown when the collapse springs finish. architecture:react-component-runtime INV3 asks a new effect to name the external system it synchronizes; a comment saying it waits for the collapse motion's finished promises would cover it.
c79ed6b to
79c4b3c
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on direction no current spec covers: draft spec:AST-094 FR1–FR23 with DEC-1 and DEC-2, and the four ListInput decisions the description marks as pending owner review. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Whatever that decision is, these still need changes:
- Land or retarget the stack so this diff is only the ListInput change; it now carries 34,443 authored lines, most of them from other pull requests.
- Add the real-browser test that
useListReorder.tsandListReorder.test.tsxpoint to (ListReorder.interaction.browser.spec.ts), covering the slide, autoscroll, drop and cancel. - Remove the document touch listeners
ListReorderHandleinstalls once no handle needs them, and fix AST-094's references to aspec:AST-090that does not exist.
[Automated review]
|
On the predecessor stack: this pull request is stacked, as its first line says, and only its last commit (79c4b3c, 5 files, +359 −1,392) is its own. It rebases onto main as each predecessor lands, and the diff then shrinks to that commit. |
79c4b3c to
dd38d7d
Compare
dd38d7d to
969b566
Compare
969b566 to
9bbfef4
Compare
9bbfef4 to
e75eada
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on direction no current spec covers: draft spec:AST-094 FR1–FR23 with DEC-1 and DEC-2 (including whether FR4's untunable touch motion or the themable reorder.slide.spring stands) and the ListInput reorder decisions the description marks as pending owner review, now including the handle's moves menu. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision is, these still need changes:
- Land or retarget the stack so this diff is only the ListInput change; it now carries 48,303 authored lines, most of them from ten other open pull requests.
- The reorder handle announces itself as a menu button, but Enter and Space pick the record up and never open its moves menu; give that role its keyboard activation or drop
aria-haspopup. - Add a browser test for the moves menu's focus path and the drag autoscroll, and correct the theme template's
platformaxis, the stale sentence in thetouch-motion-theme-roleschangeset, and AST-094's references to aspec:AST-090that does not exist.
[Automated review]
| aria-haspopup="menu" | ||
| aria-expanded={menu === 'open'} |
There was a problem hiding this comment.
aria-haspopup="menu" and aria-expanded make this handle a menu button for assistive technology, but Enter and Space never open the menu: useListReorder.ts takes both for the pick-up and prevents their default (lines 1018–1029), so only a click reaches setMenu('mounting'). spec:AST-020 FR15 asks a part that plays an APG role for "activation by Tab plus Enter or Space". Letting Enter open the moves menu while Space keeps the pick-up, or dropping these two attributes, would resolve it.
| useEffect(() => { | ||
| if (menu !== 'mounting') { | ||
| return; | ||
| } | ||
| const frame = requestAnimationFrame(() => setMenu('open')); | ||
| return () => cancelAnimationFrame(frame); | ||
| }, [menu]); |
There was a problem hiding this comment.
This effect sets menu to 'open' on the next frame, and its comment does not name the external system it waits for (architecture:react-component-runtime INV3). Saying it waits a frame for the DropdownMenu's anchor to mount before it can move focus would cover that. The focus move into the menu and back to the handle is also browser-owned behavior with only a jsdom test (ListReorder.test.tsx: "jsdom never shows the popover"); spec:AST-013 FR10 asks for a real-browser proof of it.
| * are primary-pointer precision, contrast preference, motion preference, and | ||
| * platform (`ios` for the browsers of iPhone and iPad, `other` for the rest). |
There was a problem hiding this comment.
Adaptation rules have no platform axis: defineTheme refuses when: {platform: 'ios'} (the head's own test "refuses a platform condition, which adaptations do not have" in expandMotionRoles.test.ts). A theme author following this template comment gets a thrown error; dropping the platform sentence fixes it.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Themes can retune touch motion roles. `defineTheme({motion})` takes `springs`, `durations` and `curves` keyed by motion role, the table path of a measured touch motion value (`springs.sheetPresent`, `navigation.transitionMs`), at the root or in an adaptation rule, and writes one `--motion-*` variable per role it sets. A role the theme leaves out keeps its measured default, an unknown role id is refused, and the duration scale (`fast`, `medium`, `ratio`) is optional when a theme sets only roles. No shipped component reads the roles yet. `astryx docs motion` lists every motion role with its CSS variable and default, and the theme authoring guide and template describe the new fields. |
There was a problem hiding this comment.
"No shipped component reads the roles yet" no longer holds in this branch: List (useListUpdateMotion.ts), TreeList (useTreeExpansionMotion.ts), the selection hold and the reorder (ListReorderTouch.ts) all read motion roles, as the list-selection-hold and tree-list-expand-motion changesets say. Dropping the sentence keeps the release note accurate.
e75eada to
148a984
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on direction no current spec covers: draft spec:AST-094 FR1–FR23 with DEC-1 and DEC-2 (including whether FR4's untunable touch motion or the themable reorder.slide.spring stands) and the ListInput reorder decisions the description marks as pending owner review, including the handle's new look and drag for mouse users. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision is, these still need changes:
- Land or retarget the stack so this diff is only the ListInput change; it now carries 48,192 authored lines, most of them from nine other open pull requests.
- The reorder handle announces itself as a menu button, but Enter and Space pick the record up and never open its moves menu; give that role its keyboard activation or drop
aria-haspopup. - Add browser evidence for the moves menu's focus path, the drag autoscroll and the mouse drag after the change, and mark or drop the new
@astryxdesign/core/motion/reducedMotion.stylexexport. - Correct the changesets that say no component uses the gesture and motion code yet, and AST-094's references to a
spec:AST-090that does not exist.
[Automated review]
| aria-label={name} | ||
| aria-describedby={context.hintId} | ||
| aria-disabled={isDisabled || undefined} | ||
| aria-haspopup="menu" |
There was a problem hiding this comment.
aria-haspopup="menu" and aria-expanded make this handle a menu button, but Enter and Space never open the menu: useListReorder.ts takes both for the pick-up and prevents their default (lines 1018–1028), and a prevented Enter or Space keydown suppresses a button's click in Chromium, so only a pointer click reaches setMenu('mounting'). spec:AST-020 FR15 asks a part that plays an APG role for "activation by Tab plus Enter or Space". Letting Enter open the moves menu while Space keeps the pick-up, or dropping these two attributes, would resolve it.
| "types": "./dist/utils/interactionOverlay.stylex.d.ts", | ||
| "default": "./dist/utils/interactionOverlay.stylex.js" | ||
| }, | ||
| "./motion/reducedMotion.stylex": { |
There was a problem hiding this comment.
This adds a public @astryxdesign/core/motion/reducedMotion.stylex subpath outside any /experimental boundary, and reducedMotion.stylex.ts carries no @experimental, so under spec:AST-017 FR21 its first release makes reducedMotionFadeConsts and reducedMotionKeys stable API; the touch-experimental-subpaths changeset also says no other subpath is added. Moving them under motion/experimental or keeping them internal would fix it.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Themes can retune touch motion roles. `defineTheme({motion})` takes `springs`, `durations` and `curves` keyed by motion role, the table path of a measured touch motion value (`springs.sheetPresent`, `navigation.transitionMs`), at the root or in an adaptation rule, and writes one `--motion-*` variable per role it sets. A role the theme leaves out keeps its measured default, an unknown role id is refused, and the duration scale (`fast`, `medium`, `ratio`) is optional when a theme sets only roles. No shipped component reads the roles yet. `astryx docs motion` lists every motion role with its CSS variable and default, and the theme authoring guide and template describe the new fields. |
There was a problem hiding this comment.
"No shipped component reads the roles yet" does not hold in this branch: the reorder (ListReorderTouch.ts), useListUpdateMotion.ts, useTreeExpansionMotion.ts and the selection hold read motion roles. The [chore] notes for the gesture host, tap and long press, the animator, the motion math and the reorder values say the same about code List now runs. Dropping those sentences keeps the release notes accurate.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
There is no spec:AST-090 on main or at this head, so this proposed amendment, and the one in Current-state impact, has no record for its owners to rule on. Cite the record that states these exit rules, or drop the amendment.
| * | ||
| * Under a finger only (a coarse primary pointer): a fine pointer keeps the | ||
| * variant exactly, with no layer, no stacking context and its own touch | ||
| * paint (`spec:AST-017` FR55, touch work leaves desktop as it was). |
There was a problem hiding this comment.
On main the claim that touch work leaves fine-pointer behavior alone is spec:AST-017 FR59; there is no FR55. The same citation appears in interactionOverlay.cell.test.ts and ListRowPressPaths.interaction.browser.spec.ts.
… 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.
Approval waits on an engineering owner's decision on ListInput's new reorder handle: a 28 px three-line button after Remove that mouse and keyboard users also get, which no current spec decides and spec:AST-017 FR59 sends to its own owner decision. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever the decision, these need changes:
- Reorder mode ends after the first move and announces the old position names, because each handle is named by its index (
ListInput.tsx:1195,spec:AST-103FR4). - ListInput's list never applies the reorder session's list style, so no insertion line shows for a drag or a key move (
ListInput.tsx:1024-1029). - Each record gains two elements (FR5), the handle's code loads without
reorderable(FR2), its name is concatenated (architecture:internationalizationINV1a), and the docs describe behavior this head does not have. - Land or retarget the stack: the diff carries 44,231 authored lines, almost all from other open pull requests.
[Automated review]
| {hasHandle ? ( | ||
| // Named "Reorder guest 2", like Remove. | ||
| <ListReorderHandle | ||
| label={`${itemName} ${index + 1}`} |
There was a problem hiding this comment.
Naming the handle by its index ends reorder mode on every committed move: the new label re-runs the handle's registration, and its cleanup ends the mode (listReorderEngine.tsx:662-669); the move is also announced with the old names. spec:AST-103 FR4 keeps the mode until Escape, Done or the handle. Keep the handle registered when only its name changes, and build the name from a catalog message (@astryx.listInput.reorderItem is still in en.json) instead of concatenating it (architecture:internationalization INV1a).
| role="list" | ||
| aria-labelledby={labelID} | ||
| aria-describedby={joinIDs(descriptionID, statusID)} | ||
| {...stylex.props(styles.list)}> |
There was a problem hiding this comment.
This list never takes reorderScope.session.listStyle, the rule that draws the insertion line (core List applies it at List.tsx:371), so neither a drag nor a key move shows where the record will land (design:ordered-collection-reordering DR3 and DEC-1, spec:AST-103 FR4).
| {showReorderHandle ? ( | ||
| <div | ||
| {...mergeProps( | ||
| // Revealed like Remove; it keeps its column, so | ||
| // the fields never shift. | ||
| getContentRevealProps({isLayoutPreserved: true}), | ||
| stylex.props( | ||
| styles.controlCell, | ||
| styles.reorderControlCell, | ||
| ), | ||
| )}> | ||
| {hasHandle ? ( | ||
| // Named "Reorder guest 2", like Remove. | ||
| <ListReorderHandle | ||
| label={`${itemName} ${index + 1}`} | ||
| itemKey={itemKey} | ||
| isDisabled={isDisabled || isLoading} | ||
| /> | ||
| ) : null} | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
The cell <div> plus the handle adds two elements to each record, and a record isReorderable rejects keeps an empty cell; spec:AST-103 FR5 allows one added element, the handle.
| import { | ||
| ListReorderHandle, | ||
| ListReorderScope, | ||
| useListReorderScope, | ||
| } from '@astryxdesign/core/internal/reorder'; |
There was a problem hiding this comment.
Importing ListReorderHandle statically puts reorder code in every ListInput bundle, with or without reorderable (spec:AST-103 FR2). Core List renders reorder.session.Handle from the object instead (ListItem.tsx:444).
| description: | ||
| 'Shows a vertical grip beside the first field of each record. The grip is a bare six-dot glyph with no button fill. It hangs in the inline-start gutter, outside the fields, so the fields and their labels never move, and it appears while the record is hovered or holds keyboard focus and stays visible on touch screens. Grip and clearance take 16px, so the grip sits inside the standard 16px container padding; give the list that much free inline-start space in a clipping or edge-to-edge container. During pointer reordering, list rows stay stationary while the source row and a free-moving duplicate that follows the pointer on both axes appear at 50% opacity as a temporary drag-only state; full opacity returns immediately on drop or cancel. Vertical pointer position determines the accent insertion line and final list placement. The controlled order commits and affected rows animate with Astryx motion tokens only on release. When the grip is focused, Arrow Up or Arrow Down immediately moves the record one position. Space or Enter starts lift mode for Arrow, Home, or End previewing; Space or Enter drops and Escape cancels. Focus and live announcements are preserved.', | ||
| default: 'false', | ||
| "Makes the records reorderable: pass the object createReorderable makes, keyed by getItemKey. Each record ends with core List's reorder handle, after Remove, as UIKit's reorder control ends an editing row. The handle appears while the record is hovered or holds keyboard focus and stays visible on touch screens. On a touch screen, hold the handle for a moment, then drag: the record follows the finger, the records it passes slide out of its way, the list autoscrolls near the edges, and the drop springs the record into place; a flick on the handle scrolls and a pinch zooms the page. A mouse drags at once, without motion. Activating the handle (a tap, a click, Space or Enter) turns on reorder mode for its record, in which each arrow key moves it a place and Escape or Done ends the mode, keeping the moves. The object's isReorderable(key) says which records move: one it rejects has no handle. Each move goes to the object's onReorder({key, to}), to being the record's index in value after the move. In a disabled or loading list the handles stay focusable but move nothing. Without it, the records keep their order.", |
There was a problem hiding this comment.
The records a drag passes no longer slide aside: the engine keeps them still with a preview and an insertion line, as the current reordering design's DEC-1 requires. The anatomy entry also claims a 44 px target, Home and End, and a Space/Escape pick-up, the dense guidance promises one onChange per move, and the anatomy omits the reorder hint that spec:AST-103 FR4 requires.
…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.
Approval waits on an engineering owner deciding, or identifying the governing spec for, ListInput's new reorder handle: a 28 px three-line button after Remove that mouse and keyboard users also get, which no current spec decides and spec:AST-017 FR59 sends to its own owner decision. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever the decision, these need changes:
- Reorder mode ends after the first move and announces the old position names, because each handle is named by its index (
ListInput.tsx:1195,spec:AST-103FR4). - ListInput's list never applies the reorder session's list style, so no insertion line shows for a drag or a key move (
ListInput.tsx:1024-1029). - Each record gains two elements (FR5), the handle's code loads without
reorderable(FR2), its name is concatenated (architecture:internationalizationINV1a), and the docs and the design record's deleted gap note describe behavior this head does not have. - Land or retarget the stack: the diff carries 44,560 authored lines, almost all from other open pull requests.
[Automated review]
| {hasHandle ? ( | ||
| // Named "Reorder guest 2", like Remove. | ||
| <ListReorderHandle | ||
| label={`${itemName} ${index + 1}`} |
There was a problem hiding this comment.
Naming the handle by its index ends reorder mode on every committed move: the new label re-runs ListReorderHandle's registration effect, and its cleanup ends the mode (listReorderEngine.tsx:662-668); the move is also announced with the names from before it. spec:AST-103 FR4 keeps the mode until Escape, Done or the handle, so that the data, the screen and the announcement agree. The name is also assembled here rather than from the catalog (architecture:internationalization INV1a).
| <ol | ||
| ref={listRef} | ||
| role="list" | ||
| aria-labelledby={labelID} | ||
| aria-describedby={joinIDs(descriptionID, statusID)} | ||
| {...stylex.props(styles.list)}> |
There was a problem hiding this comment.
This list never takes reorderScope.session.listStyle, the rule that draws the insertion line (core List applies it at List.tsx:371), so neither a drag nor a key move shows where the record will land (design:ordered-collection-reordering DR3, DR7 and DEC-1; spec:AST-103 FR4).
| </div> | ||
| ) : null} | ||
| {showReorderHandle ? ( | ||
| <div |
There was a problem hiding this comment.
This cell plus the handle inside it adds two elements to each record, and a record isReorderable rejects keeps the empty cell; spec:AST-103 FR5 allows one added element, the handle.
| import { | ||
| ListReorderHandle, | ||
| ListReorderScope, | ||
| useListReorderScope, | ||
| } from '@astryxdesign/core/internal/reorder'; |
There was a problem hiding this comment.
Importing the handle and scope statically puts reorder code in every ListInput bundle, with or without reorderable (spec:AST-103 FR2: a collection imports none of it until the object is passed).
| @@ -219,7 +218,7 @@ export const docs = { | |||
| name: 'Reorder handle', | |||
| required: false, | |||
| description: | |||
| "A bare six-dot grip beside the first field of the record, with an accessible label and no hover tooltip or button fill. Its target is 8px wide and as tall as the field, with 8px of clearance to the field, which keeps it within WCAG 2.5.8 through the spacing exception; the glyph uses the primary icon color and shows the standard focus ring. It hangs in the inline-start gutter, outside the fields, so the fields and their labels keep their positions, and grip plus clearance fit the standard 16px container padding. It is revealed while the record is hovered or contains keyboard focus and stays visible on touch screens. At rest it is transparent but stays in the tab order. While a record is being reordered its grip stays visible, and a pointer drag keeps the other records' grips hidden. In the stacked layout, the grip hangs beside the first field and Remove sits on the other side of that field at the top of the record. Pointer dragging keeps the list stationary, fades both source and duplicate preview to 50%, lets the duplicate follow freely on both axes, and uses vertical position for the insertion line before committing and animating on release. A focused grip moves one position with Arrow Up or Arrow Down; Space or Enter also enables lift mode with Arrow, Home, End, drop, and Escape controls.", | |||
| "Core List's reorder handle, three lines at the end of the record after Remove, with an accessible label naming the record and its position and no hover tooltip. Its target is at least 44 by 44 px. It is revealed while the record is hovered or contains keyboard focus and stays visible on touch screens; at rest it is transparent but stays in the tab order. In the stacked layout it sits at the top of the record beside Remove. A finger holds it a moment, then drags; a mouse drags at once; a focused handle moves the record with Arrow Up, Arrow Down, Home and End, and Space and Escape pick it up and put it back.", | |||
There was a problem hiding this comment.
At this head the handle is 28 × 28 px for a mouse, not “at least 44 by 44 px”; Home and End are not handled, and Space and Escape toggle and end reorder mode, keeping the moves. The anatomy also needs the reorder-hint part spec:AST-103 FR4 asks each adopting collection to name, and lines 98 and 613 still describe records sliding aside.
| Implementation gap: Lab's ListInput and TransferList draw no insertion line | ||
| when reordering by keyboard (DR7); ListInput's keyboard grab mode moves the | ||
| rows before the commit (DR3); and both build their own drag preview | ||
| (`architecture:component-style-authoring` INV7). |
There was a problem hiding this comment.
The DR7 part of this note still holds for ListInput at this head: its list draws no insertion line in keyboard reorder mode. Keeping that sentence until the line is drawn keeps the record accurate.
|
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 #8143. Review only this PR's change: vjeux/astryx@a6bcd6d...fe0bb94
Why
ListInput lets people build an ordered list of records, such as guests. Its grip grabbed every touch at once, so swiping over grips on a phone didn't scroll.
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. ListInput had its own drag code, duplicating core
List's reorder. Following Cindy's direction (her 1272d6a is here), it takesreorderable, thecreateReorderableobject, keyed bygetItemKey. Each record ends with core's handle ("Reorder guest 2"), each move goes toonReorder({key, to}), and disabled or loading lists keep their handles inert. Lab reaches core's reorder through@astryxdesign/core/internal/reorder.Before:
<ListInput isReorderable onChange={...} />. After:<ListInput reorderable={createReorderable({onReorder})} getItemKey={...} />.Options and tradeoffs
Why this choice
Reusing core's reorder means one implementation, and ListInput gets the hold, the keyboard path and the announcements. The cost is the API change; Lab is private, so there's no changeset. The handle goes after Remove, where UIKit puts it. ListInput shrinks by 4,181 B gzip. The design record drops the removed
reorderStyles.tspath and its "Implementation gap" paragraph, which this closes, for Cindy's review.How to check it
In Storybook
Lab/ListInput:I ran the unit tests and the Chromium browser tests, covering moves through
onReorder, keyboard reorder mode, inert handles, and pinch-zoom on a handle. All pass. Two reds come from elsewhere:touch-support-bundle, which #8671 fixes, andpr-rtl, main's newer RTL test from 0472c4e.Writing rubric results
For
design:ordered-collection-reordering:Evidence
Before: the grip drags at once, and on iOS 26.2: after, hold, drag and drop, and loading: the grip is inert.
Stack (merge in this order): … → #8139 → #8337 → #8143 → this.