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
19c90f7 to
f564f48
Compare
meta-cla
Bot
added
the
CLA Signed
![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 a spec decision: no current record admits List's new editingControl="select" (with the isEditing mode it renders in), so an engineering owner needs to admit it as stable component:List API or route it through AST-017's experimental prop object, and record that by commenting /approve-spec <full head SHA> on this pull request once a record in it states the prop; the author may do so as an engineering owner. Independent of that decision, these changes are needed:
Itemnow gives delegated rows the list-row press layer, so a finger paints the wholeRadioListrow andRadioList.test.tsx:933fails in CI; keep the layer off delegated rows.- The edit-mode selection reaches assistive technology only as
aria-currenton each selected row; expose it on the row's button, for example witharia-pressed. - Export
ListEditingControlfromList.tsxandList/index.ts, and align draft AST-094 FR8 with the current reduced-motion record's opacity-only cross-fade of 200 ms or less. - The diff carries the whole 86-commit stack (about 40,000 authored lines); retarget this pull request once the earlier ones land, so it carries only the edit-mode selection commit.
[Automated review]
| // The touch press on a compositor layer (after the overlay, whose | ||
| // own touch paint it turns off): the release keeps fading while | ||
| // script runs. | ||
| isInteractive && interactionOverlayStyles.cell, |
There was a problem hiding this comment.
isInteractive includes rows that delegate through interactiveRef (line 619), so RadioListItem and other delegating rows get this cell layer too. Under a finger its ::before fills the whole option row and the row's release animation is turned off; the current user-states design record says only the RadioList indicator paints, and CI's RadioList.test.tsx:933 fails on it. Apply the layer (and cellPressableProps at line 889) only to rows that paint their own press.
| aria-hidden="true" | ||
| {...{[EDITING_CONTROL_ATTRIBUTE]: ''}} | ||
| {...stylex.props(editingStyles.control)}> | ||
| <CheckboxControl |
There was a problem hiding this comment.
The indicator is aria-hidden, and the row's button carries no checked or pressed state; selection reaches assistive technology only as aria-current="true" on the li, on every selected row at once. While editingControl="select" is in edit mode, consider aria-pressed={isSelected} on the row's button instead.
| * slides in with the rows and fades in, and slides out on leaving. | ||
| * Proposed public API, pending owner review. | ||
| */ | ||
| editingControl?: ListEditingControl; |
There was a problem hiding this comment.
ListEditingControl types this public prop but is not re-exported here (lines 43-47) or from List/index.ts, so callers cannot import it by name.
| on: a transition that travels becomes a cross-fade carrying the same change | ||
| of state. An entrance lands in place and fades in, an exit fades out in | ||
| place, and any other move fades through. A navigation push or pop | ||
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen |
There was a problem hiding this comment.
A 0.5 s cross-fade (and spring-timed fades for sheets and menus) under Reduce Motion contradicts the current reduced-motion design record, DR2: opacity only, 200 ms or less. The stack's animator already follows DR2.
f564f48 to
6e79897
Compare
6e79897 to
735dc2c
Compare
There was a problem hiding this comment.
Approval waits on a spec decision: no current record admits List's new editingControl="select" or the isEditing edit mode it renders in, so an engineering owner needs to admit them as stable component:List API (including the control's mouse presentation, how selection reaches assistive technology, and whether CheckboxList already covers the need) or route them through AST-017's experimental prop object, and record that by commenting /approve-spec <full head SHA> on this pull request once a record states the prop; the author may do so as an engineering owner. Independent of that decision, these changes are needed:
- Export
ListEditingControlfromList.tsxandList/index.ts, since callers cannot import the prop's type today. - Expose the edit-mode selection on the row's button (for example with
aria-pressed) instead of onlyaria-currenton each selected row, which gives screen-reader users no checked state. - Drop "Proposed public API, pending owner review." from the JSDoc and the consumer doc once the decision is made.
- The diff still carries the whole 101-commit stack (about 39,700 authored lines), so retarget this pull request once the earlier ones land and let it carry only the edit-mode selection commit.
[Automated review]
| * slides in with the rows and fades in, and slides out on leaving. | ||
| * Proposed public API, pending owner review. | ||
| */ | ||
| editingControl?: ListEditingControl; |
There was a problem hiding this comment.
ListEditingControl types this public prop but is declared only in the internal ListContext.tsx; neither the re-export at lines 40-44 nor List/index.ts includes it, so callers cannot import it from @astryxdesign/core.
| (isEditing || (ctx?.showsEditingControl ?? false)); | ||
| const editingControl = showsControl ? ( | ||
| <CheckboxControl | ||
| aria-hidden="true" |
There was a problem hiding this comment.
The indicator is aria-hidden and the row's button carries no pressed or checked state, so in edit mode the selection reaches assistive technology only as aria-current="true" on each selected li. While editingControl="select" is in edit mode, consider aria-pressed={isSelected} on the row's button.
| * and its one tab stop, and activating the row calls its `onClick` as | ||
| * before, for the app to toggle the row's selection. Under a finger it | ||
| * slides in with the rows and fades in, and slides out on leaving. | ||
| * Proposed public API, pending owner review. |
There was a problem hiding this comment.
This sentence ships in stable JSDoc (and in List.doc.mjs); it does not mark the prop experimental, and it is wrong once the prop merges as stable. Drop it, or replace it with AST-017's experimental boundary, once the owner decides.
735dc2c to
dc4a27b
Compare
There was a problem hiding this comment.
Approval waits on a spec decision: no current record admits List's new editingControl="select" or the isEditing edit mode it renders in, so an engineering owner needs to provide or identify the governing spec, either admitting them as stable component:List API (including the checkbox column's mouse presentation, how selection reaches assistive technology, and whether CheckboxList already covers the need) or routing them through AST-017's experimental prop object, and record that by commenting /approve-spec <full head SHA> on this pull request once a record states the prop; the author may do so as an engineering owner. Independent of that decision, these changes are needed:
- The diff still carries the whole 101-commit stack (about 39,900 authored lines), so retarget this pull request once the earlier ones land and let it carry only the edit-mode selection commit.
- Expose the edit-mode selection on the row's button (for example with
aria-pressed) instead of onlyaria-currenton each selected row, which gives screen-reader users no checked state. - Export
ListEditingControlfromList.tsxandList/index.ts, since callers cannot import the prop's type today. - Drop "Proposed public API, pending owner review." from the JSDoc and the consumer doc once the decision is made.
[Automated review]
There was a problem hiding this comment.
Approval waits on a spec decision: no current record admits List's new editingControl="select" or the isEditing edit mode it renders in, so an engineering owner needs to provide or identify the governing spec, either admitting them as stable component:List API (including the checkbox column's mouse presentation, how the selection reaches assistive technology, and whether CheckboxList already covers the need) or routing them through AST-017's experimental prop object, and record that by commenting /approve-spec <full head SHA> on this pull request once a record states the prop; the author may do so as an engineering owner. Independent of that decision, these changes are needed:
- The diff still carries the whole 111-commit stack (about 39,900 authored lines), so retarget this pull request once the earlier ones land and let it carry only the edit-mode selection commit.
- Expose the edit-mode selection on the row's button (for example with
aria-pressed) instead of onlyaria-currenton each selected row, which gives screen-reader users no checked state. - Export
ListEditingControlfromList.tsxandList/index.ts, since callers cannot import the prop's type today. - Drop "Proposed public API, pending owner review." from the JSDoc and the consumer doc once the decision is made.
[Automated review]
| (isEditing || (ctx?.showsEditingControl ?? false)); | ||
| const editingControl = showsControl ? ( | ||
| <CheckboxControl | ||
| aria-hidden="true" |
There was a problem hiding this comment.
The indicator is aria-hidden and the row's button carries no pressed or checked state, so in edit mode the selection reaches assistive technology only as aria-current="true" on each selected li. While editingControl="select" is in edit mode, consider aria-pressed={isSelected} on the row's button, and update ListEditingSelect.test.tsx to match.
| @@ -15,3 +16,16 @@ export {List} from './List'; | |||
| export type {ListProps, ListStyle, ListDensity, ListEdge} from './List'; | |||
There was a problem hiding this comment.
ListEditingControl types the new public editingControl prop but is declared only in the internal ListContext.tsx; add it here and to the re-export in List.tsx so callers can import it from @astryxdesign/core.
| * and its one tab stop, and activating the row calls its `onClick` as | ||
| * before, for the app to toggle the row's selection. Under a finger it | ||
| * slides in with the rows and fades in, and slides out on leaving. | ||
| * Proposed public API, pending owner review. |
There was a problem hiding this comment.
This sentence ships in stable JSDoc (and in List.doc.mjs); it does not mark the prop experimental, and it is wrong once the prop merges as stable. Drop it, or replace it with AST-017's experimental boundary, once the owner decides.
There was a problem hiding this comment.
Approval waits on a spec decision: no current record admits List's new editingControl="select" or the isEditing edit mode it renders in, so an engineering owner needs to provide or identify the governing spec, either admitting them as stable component:List API (including the checkbox column's mouse presentation, how the selection reaches assistive technology, and whether CheckboxList already covers the need) or routing them through AST-017's experimental prop object, and record that by commenting /approve-spec <full head SHA> on this pull request once a record states the prop; the author may do so as an engineering owner. Independent of that decision, these changes are needed:
- The diff still carries the whole stack (about 40,500 authored lines), so retarget this pull request once the earlier ones land and let it carry only the edit-mode selection commit.
- Expose the edit-mode selection on the row's button (for example with
aria-pressed) instead of onlyaria-currenton each selected row, which gives screen-reader users no checked state. - Export
ListEditingControlfromList.tsxandList/index.ts, since callers cannot import the prop's type today. - Keep edit mode's room when a theme sets the row's inline padding: under an
itempaddingInlineoverride the checkbox covers the start of each label. - Drop "Proposed public API, pending owner review." from the JSDoc and the consumer doc once the decision is made.
[Automated review]
| (isEditing || (ctx?.showsEditingControl ?? false)); | ||
| const editingControl = showsControl ? ( | ||
| <CheckboxControl | ||
| aria-hidden="true" |
There was a problem hiding this comment.
The indicator is aria-hidden and the row's button carries no pressed or checked state, so in edit mode the selection reaches assistive technology only as aria-current="true" on each selected li (every selected row at once). While editingControl="select" is in edit mode, consider aria-pressed={isSelected} on the row's button, and update ListEditingSelect.test.tsx to match.
| row: { | ||
| paddingInlineStart: { | ||
| default: 'var(--_item-inset-inline)', | ||
| '[data-astryx-editing]': `calc(var(--_item-inset-inline) + ${listRowConsts.editingIndent})`, |
There was a problem hiding this comment.
This indent lives in the row's own padding-inline-start, which a theme's item paddingInline override replaces (theme rules sit in the layer above StyleX's). Under defineTheme({components: {item: {base: {paddingInline: '16px'}}}}) the row keeps 16 px in edit mode and the checkbox, still placed from --_item-inset-inline, covers the first letters of the label. Carrying the room on something the theme's padding does not replace, and placing the control from that edge, avoids it.
| @@ -15,3 +16,16 @@ export {List} from './List'; | |||
| export type {ListProps, ListStyle, ListDensity, ListEdge} from './List'; | |||
There was a problem hiding this comment.
ListEditingControl types the new public editingControl prop but is declared only in the internal ListContext.tsx; add it here and to the re-export in List.tsx so callers can import it from @astryxdesign/core.
| * and its one tab stop, and activating the row calls its `onClick` as | ||
| * before, for the app to toggle the row's selection. Under a finger it | ||
| * slides in with the rows and fades in, and slides out on leaving. | ||
| * Proposed public API, pending owner review. |
There was a problem hiding this comment.
This sentence ships in stable JSDoc (and in List.doc.mjs); it does not mark the prop experimental, and it is wrong once the prop merges either way. Drop it, or replace it with AST-017's experimental boundary, once the owner decides.
…election hold change
…touch hit area change
…tree expansion change
…list updates change
…it mode change
…tion control change
There was a problem hiding this comment.
Approval waits on an owner decision: editingControl="select" and the isEditing edit mode it renders in are new public List API that no current spec governs, so an engineering owner needs to admit them on component:List (or route them through the experimental prop object) and records that 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, please split the stacked changes into their own pull requests and fix the following:
- With a theme that sets the item
paddingInline, the edit-mode checkbox covers the first 24 px of each row's label. hasAnimatedChangesand TreeList's expansion motion duplicate theanimatedLayouthandle thatspec:AST-105makes the one API for this and play spring constants instead of motion roles (spec:AST-094FR4), edit-mode selection is exposed only asaria-current, andListEditingControlis not exported.
[Automated review]
| row: { | ||
| paddingInlineStart: { | ||
| default: 'var(--_item-inset-inline)', | ||
| '[data-astryx-editing]': `calc(var(--_item-inset-inline) + ${listRowConsts.editingIndent})`, |
There was a problem hiding this comment.
A theme item.paddingInline override (allowed by component:List FR7) sets padding-inline in the astryx-theme layer above this rule, so in edit mode the row keeps the theme's padding with no 40 px room while the control is still placed from --_item-inset-inline: with a 12 px override the 24 px checkbox covers the first 24 px of every label. Carry the room on a property the theme layer does not set, and add a theme-override case to ListEditingSelect.test.tsx.
| (isEditing || (ctx?.showsEditingControl ?? false)); | ||
| const editingControl = showsControl ? ( | ||
| <CheckboxControl | ||
| aria-hidden="true" |
There was a problem hiding this comment.
With the control aria-hidden, the only state a selected row exposes in edit mode is aria-current="true" (Item), so several rows are announced as current and the checked state is not programmatically available (spec:AST-020, 4.1.2). Expose the selection with semantics that mean selected, and test it.
| * Off by default; with a mouse nothing moves. | ||
| * @default false | ||
| */ | ||
| hasAnimatedChanges?: boolean; |
There was a problem hiding this comment.
spec:AST-105 makes the animatedLayout handle the one API for animating content around an inserted or removed item (FR1, FR6: not a visual boolean), on springs.settle (FR4) under every pointer (FR5). This boolean is a second, touch-only path on a per-component spring; split it out, or adopt the animatedLayout prop under List's record.
| /** Marks a row's trailing content (Item's end slot, Item/Item.tsx). */ | ||
| const TRAILING_ATTRIBUTE = 'data-astryx-item-end'; | ||
|
|
||
| const SPRING = list.update.tableSpring; |
There was a problem hiding this comment.
list.update.tableSpring is declared "no motion role" (motion/uikit/list.ts), but spec:AST-094 FR4 requires every touch motion to be read through a motion role and forbids playing a literal. The same applies to useListUpdateMotion.ts and useTreeExpansionMotion.ts.
| export const EDITING_ATTRIBUTE = 'data-astryx-editing'; | ||
|
|
||
| /** The control edit mode shows in each row's leading column. */ | ||
| export type ListEditingControl = 'select'; |
There was a problem hiding this comment.
The description promises ListEditingControl as a new public type, but neither List.tsx nor List/index.ts re-exports it, so callers cannot import it from @astryxdesign/core/List (architecture:public-component-api INV1).
| * and its one tab stop, and activating the row calls its `onClick` as | ||
| * before, for the app to toggle the row's selection. Under a finger it | ||
| * slides in with the rows and fades in, and slides out on leaving. | ||
| * Proposed public API, pending owner review. |
There was a problem hiding this comment.
This sentence ships to consumers in the JSDoc and in List.doc.mjs, and is wrong after any decision; drop it, or use the experimental prop object if owners choose that route.
| // A selection hold returns focus to this row after a pop; its ring follows | ||
| // how the user last interacted (List/selectionHold.ts). | ||
| useInteractionModalityTracking(); | ||
| const {showsChildren, groupRef} = useTreeExpansionMotion(isExpanded); |
There was a problem hiding this comment.
Under a finger this animates expand and collapse, with the rows below gliding, without an animatedLayout handle; spec:AST-105 FR2 says a component without a handle keeps its behavior and content after the change takes its place at once, and names TreeList as the handle's first adopter.
…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
…it area; the target never drops below 44 px) into the edit mode change
… hit area; the target never drops below 44 px) into the list updates change
…area; the target never drops below 44 px) into the selection control change
A theme's `item.paddingInline` sits in the theme's cascade layer, above every component class, so the room edit mode added to the row's leading padding through a class never applied under it: no room appeared, yet each toggle under a finger threw the text by the indent and sprang it back. The room is now an inline padding, the indent beyond the inset the row applies (the theme's when it sets one, through `--_item-inset-inline`), so the text moves by exactly the room that appears. Red first: edit-mode rows carry the room inline, under `bleed` too (unit); under a theme's 24 px row padding the text slides by the indent frame by frame (browser; it jumped 40 px before).
…o the selection control 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.
…stays clear of themed text, and its type is exported - The row's activation is a checkbox whose checked state is the row's selection (`component:List` AR3), through an internal seam between Item and the component that renders it (Item/ItemActivationContext.ts). The row's aria-current gives way to it, the indicator stays hidden from assistive technology, and the row keeps its one tab stop; outside edit mode nothing changes. - Under a theme's row padding the control no longer covers the text: the room it stands in is the edit-mode room, which now applies beyond the theme's inset. - `ListEditingControl`, the type of `editingControl`, is exported from `@astryxdesign/core` and `@astryxdesign/core/List`. Red first: each row is a checkbox named by its label, checked when selected, with no aria-current (unit); under a theme's 24 px row padding the control ends before the text (browser); the type import (typecheck).
…again on a pointer or theme change) into the tree expansion change
…again on a pointer or theme change) into the list updates change
…ain on a pointer or theme change) into the edit mode change
… on a pointer or theme change) into the selection control change
There was a problem hiding this comment.
Approval waits on an owner decision: isEditing and editingControl="select" are new public List API that no current spec governs, so an engineering owner needs to provide or identify the governing spec on component:List (covering the checkbox column a mouse sees and the checkbox semantics of each row) or route both through spec:AST-017's experimental prop object, and records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Whatever the decision, these need changing:
- The diff carries the whole open stack (about 27,000 authored lines), so please land or split the earlier changes and retarget this pull request to the selection change alone.
- In edit mode, selected
href, delegating and static rows losearia-currentand gain no checked state, while their box shows checked. hasAnimatedChangesand TreeList's expansion motion duplicatespec:AST-105'sanimatedLayout, and the edit, update and expansion motion playlist.updateconstants instead of motion roles (spec:AST-094FR4).- A mouse click in a link row's padding now follows the link, although the changeset says a mouse is unchanged (
spec:AST-017FR59). - Once the decision is made, drop “Proposed public API, pending owner review.” and update the JSDoc's description of the row's semantics.
[Automated review]
| aria-current={ | ||
| restProps['aria-current'] ?? | ||
| (isSelected && !allowsAriaSelected ? true : undefined) | ||
| (isSelected && !allowsAriaSelected && activation == null |
There was a problem hiding this comment.
In edit mode with editingControl="select", ListItem sets this context for every row, but only the onClick branch above renders the button that carries role="checkbox" and aria-checked. A selected href, interactiveRef or static row therefore loses aria-current here and gains no state of its own, while its box still shows checked (AST-020, 4.1.2). Publishing the semantics only for rows that render the button would keep the fallback for the others.
| if (target.closest('button, a, input, select, textarea')) { | ||
| return; | ||
| } | ||
| if (onClick == null && href != null && !isLinkRoot) { |
There was a problem hiding this comment.
This forwards a click in a link row's padding, marker or start and end content to the anchor under every pointer, so a mouse click there now navigates where it did nothing before, while the touch-hit-area changeset says a mouse is unchanged. AST-017 FR59 needs such a fine-pointer change named, with fine-pointer evidence, or limited to the touch hit area.
| /** Marks a row's trailing content (Item's end slot, Item/Item.tsx). */ | ||
| const TRAILING_ATTRIBUTE = 'data-astryx-item-end'; | ||
|
|
||
| const SPRING = list.update.tableSpring; |
There was a problem hiding this comment.
list.update.tableSpring is declared “no motion role” in motion/uikit/list.ts, while AST-094 FR4 says a component “MUST read touch motion through these roles and MUST NOT play a literal.” The same applies to useListUpdateMotion.ts and useTreeExpansionMotion.ts.
| * `isSelected`. It is decorative: the row keeps its selected semantics | ||
| * and its one tab stop, and activating the row calls its `onClick` as | ||
| * before, for the app to toggle the row's selection. Under a finger it | ||
| * slides in with the rows and fades in, and slides out on leaving. | ||
| * Proposed public API, pending owner review. |
There was a problem hiding this comment.
Since this head the row no longer keeps its selected semantics in edit mode: its button becomes a checkbox and aria-current goes, as List.doc.mjs already says. The last sentence also ships in the stable docs; it does not mark the prop experimental (AST-017 FR21) and is wrong after either decision.
|
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. |
|
Closing per Cindy's decision on #8687: edit experiences compose through |
Review only this PR's change: vjeux/astryx@b4e2fca...819495f (depends on #8359).
User need
An iOS table in edit mode can show a selection circle in each row to pick several; a
Listcannot.Problem and solution fit
editingControl="select"withisEditing: each row shows the theme's checkbox indicator in the room its padding makes (no wrapper), checked when the row isisSelected. It is decorative: the row keepsItem's semantics and one tab stop.list.metrics).design:reduced-motion). A mouse shows it at once (useTouchPresentation().isTouch,spec:AST-094FR13).Size (gzip, the size report's tool, base → head): List JS +252 B, CheckboxList JS +245 B, ListItem +131 B and CSS +64 B, TreeList JS +27 B.
Current specification
None governs it. Proposed public API, pending owner review (
component:List).Public delta and caller burden
<List isEditing editingControl="select">; rows passisSelectedand toggle it inonClick. New type:ListEditingControl.Evidence
Recorded on the iOS 26.2 simulator (Mobile Safari, its touch input); a real-device check is the release gate.
Scope
Testing
Written first and red: 8 unit tests, then no wrapper (unit); the browser spec covers the centre, the slide per frame, leaving, a tap toggling the mark, right to left, reduced motion and a mouse.
Green on #8076 3bfd7c3: root build and
pnpm typecheckon every head, eslint, build-css, CLI, CI guard, theuiproject's 16,109 tests on this head (load timeouts pass alone); interaction proofs at the stack's top, Chromium 91 and WebKit 74. Storycore-list--editing-selection.