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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval of this List edit mode waits on an engineering owner's decision: no current spec governs isEditing, and the draft touch-presentations spec this branch adds is unapproved. An engineering owner (the author may, as one) needs to provide or identify the component:List claim for isEditing and approve or reject the draft requirements by commenting /approve-spec <full head SHA>. Independently of that decision, these need changes:
- The diff carries the whole parent stack (about 39,000 authored lines); land or retarget the parent pull requests so this one is the edit-mode commit alone.
- Draft FR8's 0.5 s and spring-timed Reduce Motion fades contradict
design:reduced-motionDR2 (opacity only, 200 ms or less), which the animator already follows. - Draft FR4 says touch physics are not themable, but this branch adds theme motion roles that retune those springs; the two need one answer.
ListItemswitches the marker between an element and a fragment whenisEditingchanges, so bullets and numbers remount and jump on a mid-transition toggle; always render the fragment.
[Automated review]
| marker={ | ||
| editingIndent == null ? ( | ||
| marker | ||
| ) : ( | ||
| <> | ||
| {editingIndent} | ||
| {marker} | ||
| </> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Switching marker between the marker element and a fragment changes the element type in that slot, so React remounts the bullet, number or task checkbox on every isEditing toggle. With Done tapped 100 ms after Edit in a bulleted list, the text continues (−9.79 px to 30.21 px) but the new marker starts at 40.00 px instead of 33.38 px, from rest, so it jumps and trails the text. Passing the fragment in both states (marker={<>{editingIndent}{marker}</>}) keeps the marker element.
There was a problem hiding this comment.
Fixed in c1582e6: ListItem passes one fragment in both modes, so the marker keeps its slot and its element, and slides with the text. A unit test failed before the change; it checks that the same marker element survives the toggles and moves with the text.
| - **FR4 — Physics are not themable in touch presentations.** A touch | ||
| presentation MUST read its motion from the table on every theme. Themes keep | ||
| color, shape, type and spacing; the theme's duration and easing tokens keep | ||
| governing pointer presentations. **Proposed; pending owner review:** this | ||
| narrows `architecture:theme-tokens` (owners: cixzhang, imdreamrunner) to | ||
| pointer presentations, and departs from the draft `design:motion` DR3 | ||
| (owners: ernestt, cixzhang) wherever UIKit settles with an overshoot or | ||
| stretches with a rubber band. |
There was a problem hiding this comment.
FR4 says touch physics are not themable, but this branch lets defineTheme({motion}) set springs, durations and curves per role, and the animator plays the theme's value for a table spring (resolveMotion(target, spring) in motion/animator.ts), including list.update.tableSpring, which edit mode uses. The record and the code need one answer.
There was a problem hiding this comment.
This line comes from #8076, the foundation this stack sits on, not from this pull request's commit; it is addressed there.
| - **FR8 — Reduce Motion substitutes, never jumps.** Under | ||
| `prefers-reduced-motion: reduce`, a touch presentation MUST behave as its | ||
| UIKit counterpart does with Reduce Motion and Prefer Cross-Fade Transitions | ||
| 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 | ||
| modal or an alert fades on its own presentation spring and drops its travel | ||
| and scale; menus open and close on UIKit's Reduce Motion spring. Motion that | ||
| follows a finger, such as a throw, MUST keep its motion. **Proposed; pending | ||
| owner review:** this is the minimally moving form the draft `design:motion` | ||
| DR5 asks for, for touch presentations (owners: ernestt, cixzhang); the | ||
| alternative is UIKit's Reduce Motion alone, which keeps the slides of | ||
| navigation, sheets and modals. |
There was a problem hiding this comment.
The 0.5 s navigation fade and the spring-timed sheet and menu fades conflict with design:reduced-motion DR2, which requires an opacity-only transition that finishes in 200 ms or less on a coarse pointer. The animator on this branch already follows DR2 (200 ms), so FR8 should defer to that record, or the record should be amended first.
There was a problem hiding this comment.
This line comes from #8076, the foundation this stack sits on, not from this pull request's commit; it is addressed there.
e19986c to
1c127ee
Compare
meta-cla
Bot
added
the
CLA Signed
1c127ee to
c1582e6
Compare
|
On this review, at c1582e6:
|
There was a problem hiding this comment.
Approval of List's isEditing edit mode waits on an engineering owner's decision: no current spec governs the prop, and the draft touch-presentations record this branch adds (spec:AST-094) is unapproved. An engineering owner needs to provide or identify the component:List claim for isEditing (its 40 px indent, the removal of endContent, and the touch-only spring with an immediate mouse switch) and approve or reject the draft requirements, FR4 and FR8 first, by commenting /approve-spec <full head SHA> on this pull request; the author may record it as an engineering owner. Independent of that decision, these need changes:
- Reduce the diff to the edit-mode commit by landing or retargeting the parent pull requests, since it carries 39,410 authored lines today.
- Fix the
RadioList.test.tsxcase "fades the touch press out from the row, with the overlay reading its strength", which fails at this head (CI test-ui 2/4) after the press-layer change ininteractionOverlay.stylex.ts. - Make the edit-mode room without an extra element per row, or state the minimal-DOM exception with element counts.
- Reconcile FR8 with
design:reduced-motionDR2 and FR4 with the theme motion roles, and give gesture velocity one internal filter with the stillness reset.
[Automated review]
| const editingIndent = ( | ||
| <span | ||
| aria-hidden="true" | ||
| {...{[EDITING_INDENT_ATTRIBUTE]: ''}} | ||
| {...stylex.props(editingStyles.indent)} | ||
| /> |
There was a problem hiding this comment.
This adds an element to every row while editing (four in the editing story) that has no role, no themable part and wraps nothing. Current architecture:component-style-authoring INV7 says states are expressed "through attributes, classes, and styles on those elements, never through extra elements", and that an added element is justified "with measured element counts before and after". Could the 40 px room be a style on the row until a control needs the slot, or could the pull request state that exception with the counts?
| animationName: { | ||
| default: null, | ||
| [ENABLED]: {default: null, [PRESSED_FADING]: cellRelease}, | ||
| }, | ||
| animationDuration: {default: null, [PRESSED_FADING]: CELL_RELEASE}, | ||
| animationTimingFunction: { | ||
| default: null, | ||
| [PRESSED_FADING]: cellPressConsts.cellReleaseEase, | ||
| }, | ||
| animationFillMode: {default: null, [PRESSED_FADING]: 'both'}, |
There was a problem hiding this comment.
With the row's own release now kept for a fine pointer (lines 401-404), these ::before animation longhands stay unconditional, so a RadioList row's fading arm carries two animation-duration declarations. RadioList.test.tsx "fades the touch press out from the row, with the overlay reading its strength" fails at this head (CI test-ui 2/4, and locally) and passes on the base, because hasReleaseFade in __tests__/pressState.ts expects one. Gating these under the coarse query, or teaching the helper about the pointer arms, would make the suite pass again.
| * | ||
| * 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.
spec:AST-017 has no FR55; the claim that touch work leaves fine-pointer behavior as it was is FR59. The same citation appears in interactionOverlay.cell.test.ts:185 and ListRowPressPaths.interaction.browser.spec.ts:164.
| * header describes, `uikit` by default. | ||
| * @experimental | ||
| */ | ||
| export type VelocityStrategy = 'uikit' | 'lsq2' | 'iosFling' | 'window'; |
There was a problem hiding this comment.
Current architecture:interaction-modality INV10 (and spec:AST-100 FR2) asks for one internal release filter: a contact still before lifting "reads 0", the tracker "is not exported", and no Core component keeps an "alternative release filter". This tracker offers four strategies, the default never decays after a pause (lines 13-28), and PanGestureRecognizer's velocityStrategy option makes the choice reachable from @astryxdesign/core/gestures/experimental. Both records reached main after this branch's merge base, so this probably belongs in the gesture pull request's rebase.
c1582e6 to
b7a9228
Compare
There was a problem hiding this comment.
Approval of List's isEditing edit mode waits on an engineering owner's decision: no current spec governs the prop, so an engineering owner needs to provide or identify the component:List claim for it (the 40 px leading room, the removal of endContent, and the touch-only spring with an immediate mouse switch) and record that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Independent of that decision, these need changes:
- Reduce the diff to the edit-mode commit by landing or retargeting the parent pull requests, since it carries 38,438 authored lines today; the size report then becomes the base-against-head report
component-style-authoringINV6 asks for. - Keep the edit-mode room when a theme overrides
item.paddingInline, where the theme's padding wins today and the touch motion still slides the text 40 px and back. - Make the Reduce Motion dip finish within 200 ms as
design:reduced-motionDR2 requires, since its last fade starts from a timer and always ends a little later. - Give gesture velocity one release filter with the stillness reset, per
interaction-modalityINV10, in the parent pull request that adds it.
[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 that sets item.paddingInline replaces this room: theme component overrides are emitted into @layer astryx-theme, above StyleX's astryx-base (theme/Theme.tsx:99), as padding-inline itself, so the row never indents in edit mode. useListEditingMotion.ts:94-98 still shifts each part by the constant 40 px under a finger, so the text jumps and springs back to the same place. Could the room use a property a theme's row padding does not replace, or the shift come from the parts' measured movement?
| if (!landings.has(landing)) { | ||
| return; | ||
| } | ||
| landing.timer = setTimeout( |
There was a problem hiding this comment.
The dip's landing waits on this timer, and the fade back in starts only when it fires (land, line 1737), so a Reduce Motion dip ends at 200 ms plus the timer's lateness. design:reduced-motion DR2 requires each transition to finish in 200 ms or less; playing both halves as one opacity animation on the timeline, with the landing at its midpoint, would hold the bound on a busy page.
b7a9228 to
7135ed8
Compare
There was a problem hiding this comment.
Approval of List's isEditing edit mode waits on an engineering owner's decision: no current spec governs the prop, so an engineering owner needs to provide or identify the component:List claim for it (the 40 px leading room, the removal of endContent, and the touch-only spring with an immediate switch for a mouse) and record that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Independent of that decision, these need changes:
- Reduce the diff to the edit-mode commit by landing or retargeting the parent pull requests, since it carries 38,879 authored lines today.
- A theme that sets
item.paddingInlinereplaces the edit-mode room, while the touch motion still slides the text 40 px and back (ListItem.tsx,useListEditingMotion.ts). - Under Reduce Motion the dip's fade back in waits on a main-thread timer, so it can run past the 200 ms that
design:reduced-motionDR2 allows (motion/animator.ts).
[Automated review]
7135ed8 to
a4072b5
Compare
There was a problem hiding this comment.
Approval of List's isEditing edit mode waits on an engineering owner's decision: no current spec governs the prop, so an engineering owner needs to provide or identify the component:List claim for it (the 40 px leading room, the removal of endContent, and the touch-only spring with an immediate switch for a mouse) and record that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Independent of that decision, these need changes:
- Reduce the diff to the edit-mode commit by landing or retargeting the parent pull requests, since it carries 38,988 authored lines today.
- A theme that sets
item.paddingInlinereplaces the edit-mode room, while the touch motion still throws the text 40 px toward the start, where the list clips it (ListItem.tsx,useListEditingMotion.ts). - Under Reduce Motion the dip's fade back in waits on a main-thread timer, so a busy page stretches it well past the 200 ms that
design:reduced-motionDR2 allows (motion/animator.ts). - Report component sizes against the base (
component-style-authoringINV6), and give gesture velocity one release filter with the stillness reset (interaction-modalityINV10) in the gesture pull request.
[Automated review]
…election hold change
…touch hit area change
…tree expansion change
…list updates change
…it mode change
There was a problem hiding this comment.
Approval of List's new isEditing edit mode waits on a spec decision: no current spec governs it, so an engineering owner needs to provide or identify the component:List claim for it, including how it relates to reorderable's reorder mode, what its leading room does under a theme item.paddingInline override, and whether its spring becomes a List motion role. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request once that claim is in place, and the author may do so as one. Independently of that decision, these changes are needed:
- Narrow the diff to the edit-mode change, about 840 of its roughly 25,900 authored lines, by landing or dropping the pull requests below it.
- Under a theme
item.paddingInlineoverride no room appears, yet each touch toggle throws the text 40 px and springs it back (useListEditingMotion.ts:93-97); derive the shift from the offset that actually changed and add a themed test. - Play the edit spring through a motion role rather than the
list.update.tableSpringliteral, asspec:AST-094FR4 requires.
[Automated review]
…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
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).
…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
…ain on a pointer or theme change) into the edit mode change
There was a problem hiding this comment.
Approval waits on an owner decision: no current component:List claim covers isEditing (its 40 px room, hiding endContent, its relation to reorderable's reorder mode, and whether its spring becomes a List motion role), so an engineering owner needs to provide or identify that spec and record the decision by commenting /approve-spec aa928ba9e2e7c6b56bc5d12e0d8807803fcd8e56 on this pull request, which the author may also do as an engineering owner.
Independently of that decision, the edit-mode slide needs to play a motion role instead of the literal list.update.tableSpring (useListEditingMotion.ts:49), as spec:AST-094 FR4 requires.
The diff also needs to shrink to the 914-line edit-mode change by landing or splitting the stack under #8106, since it currently brings 26,148 authored lines to main.
Real-device recordings on iOS Safari and Android Chrome, which spec:AST-094 requires for touch behaviour, are still missing.
[Automated review]
| /** 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.
This plays list.update.tableSpring as a literal, and motion/uikit/list.ts declares it no motion role. CURRENT spec:AST-094 FR4 says a component "MUST read touch motion through these roles and MUST NOT play a literal". Name the edit-mode spring as a List motion role (a component's measured role joins through the generated role table, DEC-5) and play it through resolveMotion.
| {...mergeProps(themeProps('list-item'), {className, style})} | ||
| {...mergeProps(themeProps('list-item'), { | ||
| className, | ||
| style: isEditing ? {...style, ...editingRoom(edge)} : style, |
There was a problem hiding this comment.
While editing, this replaces any leading padding the caller set through style, className or xstyle with the inset plus 40 px, but useListEditingMotion.ts:97 still springs back exactly 40 px. With a 4 px caller padding the text moves 44 px and the extra 4 px jumps on the first frame. Add the indent on top of the row's applied padding, or spring back the measured change.
| * The room edit mode makes at a row's leading edge for an editing control, | ||
| * on the row itself (no element of its own): its leading padding grows by the | ||
| * indent beyond the inset it applies, so the content moves by exactly the | ||
| * indent (`component:List` FR15). It is an inline style because a theme's |
There was a problem hiding this comment.
CURRENT component:List defines FR1 to FR9, so this component:List FR15 citation (and FR10 at line 454) points at no governing text. Cite an existing claim or drop it.
|
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 |
|
Posted by Cindy's agent on her behalf. Following Cindy's decisions on the List line (dividers stay drawn on press, and no selection hold in Core), we removed both from #8106, which this PR builds on; see the note there. We can't push here because maintainer edits are off on this PR. Could you merge #8106's current head into this PR, or turn on "Allow edits by maintainers" so we can do it for you? Nothing else in the line is waiting on this one. |
|
Thanks. This one is closed, since edit mode went another way on #8687, so nothing needs merging here. The branch stays if it's ever useful. |
Review only this PR's change: vjeux/astryx@000804f...b469ed5 (depends on #8106).
User need
A
Listhas no edit mode, iOS tables' way to select, delete or reorder rows.Problem and solution fit
isEditing: UIKit's edit mode (iOS 26.2): every row indents 40 px on its own padding and itsendContentleaves.design:reduced-motion). A mouse switches at once (useTouchPresentation().isTouch,spec:AST-094FR13).Size (gzip, the size report's tool, base → head): List JS +477 B, CheckboxList JS +484 B, ListItem +119 B and CSS +60 B, TreeList JS +19 B, Item JS +13 B.
Current specification
None governs it. Proposed; pending owner review (
component:List).Public delta and caller burden
<List isEditing>, off by default. No new exports.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: 7 unit and 6 browser tests.
Green on #8076 e6ab0b9 and main 844d734: root build and
pnpm typecheckon every head, eslint, build-css, CLI, CI guard, theuiproject's 16,154 tests at the stack's top (load timeouts pass alone); interaction proofs at the stack's top, Chromium 91 and WebKit 74. Storycore-list--editing.