Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
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 an engineering owner's decision about List row height on touch screens: no current spec governs it, so an owner needs to approve, amend (for example, rows with an action only) or reject the 52 / 62 / 44 px floors under (pointer: coarse), which also reach static and Markdown lists, and name the record that holds them. 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. Two changes are needed either way:
- On a touch screen, bullets and numbers now sit 6 px above their text in marker lists and Markdown lists, because the row grows while the marker stays at its top; please keep the marker on the label's first line and cover it with a touch-context test.
- The diff carries the whole stack (14,152 authored lines); once the earlier pull requests land, rebase so this one holds only its own commit.
[Automated review]
| density === 'compact' | ||
| ? listRowStyles.touchCompact | ||
| : isRenderable(description) | ||
| ? listRowStyles.touchTwoLine | ||
| : listRowStyles.touchOneLine, |
There was a problem hiding this comment.
On a coarse pointer this floor makes the row taller than its content: Item centers the label, but the marker container keeps align-self: baseline and stays at the row's top, so bullets and numbers sit 6 px above their text (measured in core-list--bulleted-list, 40 to 52 px rows, and in Markdown, 32 to 44 px items). Keep the marker on the label's first line when the row grows, for example with the first-line slot treatment Item uses for align="first-line", and add a touch-context check that the two line up.
| /** | ||
| * A row's height on a touch screen (primary pointer coarse): UIKit's table | ||
| * rows (`list.metrics`), at least that tall, never shorter than the | ||
| * platform's control size. A theme's `min-height` on `list-item` replaces it. | ||
| */ |
There was a problem hiding this comment.
This docblock describes the touch row heights, but it sits on embeddedStyles (the divider radius reset). listRowStyles in listRow.stylex.ts already carries the same text, so this copy can go.
| }); | ||
|
|
||
| /** | ||
| * The touch presentation's query (utils/touchPresentation.ts TOUCH_QUERY), |
There was a problem hiding this comment.
utils/touchPresentation.ts does not exist at this head, so this pointer to TOUCH_QUERY leads nowhere; name the query's actual source or drop the reference.
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/CheckboxListItem | +3.7KB |
+538B |
| core/ContextMenu | +3.8KB |
+552B |
| core/ListItem | +3.8KB |
+538B |
| core/Markdown | +3.8KB |
+543B |
| core/Table | +3.8KB |
+537B |
| core/TableCell | +3.8KB |
+539B |
| core/TableHeaderCell | +3.8KB |
+552B |
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 | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 35.1KB | +546B |
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 502 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
081b45a to
e1fefc4
Compare
e1fefc4 to
ea5685d
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about List row height on touch screens: no current spec governs it, so an owner needs to approve, amend (for example, rows with an action only) or reject the 52 / 62 / 44 px floors under (pointer: coarse), which also reach static and Markdown lists, and name the record that holds them. 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. Two changes are needed either way:
- On a touch screen, list markers still sit 6 px above their text in marker lists and Markdown lists, because the row grows while the marker stays at its top; please keep the marker on the label's first line and cover it with a touch-context test.
- The diff carries the whole stack (15,116 authored lines); once the earlier pull requests land, rebase so this one holds only its own commit (the earlier line notes on
ListItem.tsxandlistRow.stylex.tsstill apply).
[Automated review]
ea5685d to
6caf11a
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about List row height on touch screens: no current spec governs the 52 / 62 / 44 px floors under (pointer: coarse), which also reach static and Markdown lists, so an owner needs to approve, amend (for example, rows with an action only) or reject them and name the record that holds them. 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. Needed either way:
- Keep list markers on the label's first line: on a touch screen they sit 6 px above their text in marker and Markdown lists; a touch-context test should hold it.
- Once the earlier stacked pull requests land, rebase so this one carries only its own commit (the earlier line notes on
ListItem.tsxandlistRow.stylex.tsstill apply).
[Automated review]
6caf11a to
34fb8e7
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about two pieces of direction no current spec covers: the 52 / 62 / 44 px List row floors under (pointer: coarse), which also reach static and Markdown lists (approve, amend or reject them and name the record that holds them), and the draft spec:AST-094 requirements FR1–FR23. 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. Needed either way:
- Keep list markers on the label's first line: on a touch screen they sit 6 px above their text in marker and Markdown lists, and a touch-context test should hold it.
- Make CI's typecheck pass: the new "Back, slowly" path in
List.stories.tsx:429handsanimatea{durationMs}timing thatHoldTimingdoes not accept (TS2345). - Once the earlier stacked pull requests land, rebase so this one carries only its own commit (the earlier line notes on
ListItem.tsxandlistRow.stylex.tsstill apply).
[Automated review]
| }; | ||
| const settle = async (to: 0 | 1, timing?: {durationMs: number}) => { | ||
| const current = hold.current; | ||
| const arrived = await current?.animate(to, timing); |
There was a problem hiding this comment.
CI's typecheck fails here (TS2345): timing is typed {durationMs: number}, which matches no member of HoldTiming, since a duration needs a curve (HoldCurveTiming). Pass {durationMs: 3000, curve: [0, 0, 1, 1]} or type the parameter as HoldTiming; if a bare duration should be supported, as the runtime already allows, make curve optional in HoldCurveTiming instead.
…Motion constant too The row press layer capped its release under reduced motion at a literal 200ms (CELL_RELEASE_REDUCED), the same cap the review on meta#8890 asked the overlay to read from the shared constant (spec:AST-094 FR4: the 200 ms Reduce Motion cap is a role, never a literal). It now reads reducedMotionFadeConsts.duration, as the overlay does; both are 200 ms, and the built stylesheet's cap is still 200 ms. A unit case holds every reduced-motion cap in the layer and the overlay to the constant. Red first: it found the layer's literal on the change before this one.
…ame window Both 'a dip held before its first frame lands once the hold is …' proofs waited for no running animation, then one frame and a timeout, assuming the next step starts within a frame. The dip lands on a timer due on the call's own clock, though, and the proofs passed only because the evaluate round trips covered the gap: in WebKit on a loaded box the window closed first and the proof read the element mid-dip (opacity 0, translate none; reproduced 1 in 4 with the round trips removed). They now wait on the call's own finished, then on the browser's own animations still running or waiting for their first frame, and read nothing from the animator.
…ment at once and settles then A Reduce Motion dip whose landing runs more than its whole fade back in late (a hold released long after the dip was due) played a 1 ms opacity animation, which WebKit keeps pending until its next frame, and settled the call on a 1 ms timer: the call's finished could resolve while the element still showed opacity 0. The animator's notes say the element appears at once there. Fails now: the landing plays one opacity animation and the call has not settled.
…element at once and settles A Reduce Motion landing that runs at least the whole fade back in late had nothing left of the cross-fade, but still played a 1 ms opacity animation and settled the call 1 ms later. WebKit keeps that animation pending until its next frame, so the call's finished could resolve with the element still at opacity 0. Such a landing now writes the end opacity at once (handing a borrowed one back to the stylesheet), plays nothing, and settles the call then, as the animator's notes say.
There was a problem hiding this comment.
Changes requested: the List touch hit area follows component:List FR10, but this pull request still carries the open #8077, #8890, #8360 and #8076 (29,461 of its 31,444 authored lines), so please land them first and rebase, or split them out. FR10's verification row also asks for every density, and an interactive density="spacious" case in the browser spec (the bottom-sheet menu rows use that density) would cover the one partition the proofs still miss.
[Automated review]
| } | ||
| }); | ||
|
|
||
| for (const story of [ |
There was a problem hiding this comment.
component:List FR10's verification row asks for every density, but this loop and the cases above open only balanced and compact lists, and the Spacious story has no interactive row. An interactive density="spacious" list (the DropdownMenu and ContextMenu bottom-sheet rows use it) with one case reading its rows' bands would close that partition; its one-line rows are 48 px, so the case should read no growth and no overlap.
There was a problem hiding this comment.
Thanks, added at 5fc6613: the Interactive story at density="spacious". Its 48 px rows reach 0 px on both sides and overlap nothing, in Chromium and WebKit; with the target raised to 56 px, the case fails.
…ot the timing table From review (an inline comment on meta#8077 about this line): the press controller, which every pressable component loads, imported the whole UIKit timing table for one value, its reduced-motion cap, while architecture:component-style-authoring INV6 ships a component only the rows of a shared table it uses. It now reads the fade's own export, reducedMotionCrossFade.durationMs, as the animator does; the value is the same 200 ms. Red first: a bundle of pressFeedback.ts carried the table's other rows (stateChangeMs, buttonReleaseMs) on the change before this one; it now carries the fade alone.
…th dividers drawn From review on meta#8077: component:List FR11's verification asks for a pressed row between two divided rows, and no case used a list with dividers. A touch case opens the Interactive story with hasDividers, holds the middle row and releases it, and checks the divider above it and its own divider below, by computed border and by the pixels of each divider's strip, while held, halfway through the release fade and after it; each strip must differ from the row's surface just inside it at rest, so a match is a drawn divider. It passes in Chromium and WebKit. A page style that hides a pressed row's divider fails it while the row is held.
…finger From review on meta#8085: component:List FR10's verification asks for every density, and the browser cases opened only balanced and compact lists. A touch case opens the Interactive story at density="spacious" (the density of the bottom-sheet menu rows): its one-line rows are 48 px, already past the 44 px target, so their layers reach 0 px on both sides and no two targets overlap. The story's density comes through a new args option of the spec's openStory helper. It passes in Chromium and WebKit. With the target raised to 56 px, it fails: the rows then grow.
… judges its first frame on that frame's time 'a spring started right before a block plays in full after it' waited a fixed 300 ms of wall clock after the block, then read the screencast. In the full interaction run on a loaded box no moving frame had reached the screencast by then, and the proof failed on an empty read. It now waits until the page shows the box a quarter of the way along and three more frames reach the screencast. Its bound changes form, not strength: the curve's own start (the handle's started) must come after the block, and the first moving frame may be no further along than a curve started a frame before the block ended, at that frame's own time, which is the old 30 px for a frame right after the block and still holds when a loaded machine sends frames sparsely. A curve dated at the call is 107 px in.
…tor's notes drop an undefined law The animator spec's header cited 'law M1' for compositor-only motion and useAnimator.ts cited 'law M4' for an animator that never re-renders, and no document defines either. The spec now cites spec:AST-094 FR5, which keeps touch motion to transform, translate, scale, rotate and opacity; useAnimator's sentence says what it means without the citation.
…e surface releaseAnimationsOf counted only the press owner's own animations (on it, or its ::before or ::after). A component that draws its release on a descendant, such as CheckboxInput's release on its box wrapper's ::after, then lost the end of it: fading went at the 200 ms clock while the 250 ms release still ran, and the frame it went relaid and repainted the box. It now also counts a CSS release animation on a descendant whose nearest press owner (closest [data-astryx-pressable]) is this element, the same nearest-owner rule the layers read by, so a press owner nested inside plays its own press's release and never extends this one. Red first: a unit case whose release runs on a box inside the row found fading gone at the clock on the change before this one; it now holds to the release's end. A second case keeps a nested press owner's release, on it or inside it, out of the row's.
There was a problem hiding this comment.
Changes requested: the List touch hit area follows component:List FR10, and the new spacious-density case completes the proofs its verification row asks for. This pull request still carries the open #8077, #8890, #8360 and #8076 (29,611 of its 31,618 authored lines), and its copy of #8360 is four commits behind that pull request's own head, so please land them first and rebase, or split them out.
[Automated review]
…ide lands when its retire runs first Reported as a WebKit hang of the held-dip proof under an 8-worker run: the proof's wait for the call's finished never settled. Instrumented, the page kept drawing frames (about 60 a second) while the element sat at opacity 0 where it started, with no animation left: the held fade out had been finished from outside, then retired (cancelled) by its timer before the frame that reads the call's start, so the call never started and its landing was never armed. This case orders the retire timer before that frame and fails on this head: the move never lands.
… that cannot come [skip ci] The retire timer cancels a finished animation once the main thread runs past its curve. When a sampler finished a call's animations before the browser started them, a loaded page could run that timer before the frame that reads the call's start: the cancel left only idle animations, the start was never read, and the call's Reduce Motion landing was never armed, so the element stayed hidden where it started and the call never settled. The retire now reads such a call's start from the finished animation before cancelling it, and with nothing left to wait on the call keeps its own clock, so its landing still runs.
There was a problem hiding this comment.
Changes requested: the List touch hit area follows component:List FR10, and its tests cover every case that record's verification row asks for. This pull request still carries the open #8077, #8890, #8360 and #8076 (29,757 of its 31,764 authored lines), so please land them first and rebase, or split them out.
[Automated review]
… curve, so only rows ship the row layer's constants The shared press overlay (utils/interactionOverlay.stylex.ts) read its release curve from the row layer's constants (cellPressConsts in rowPressLayer.stylex.ts). The overlay reaches nearly every component, and .stylex modules are side effects in core's package.json, so every component then shipped the row layer's constants (the repository's size report: +17-47 B gzip on each component without a press row), against architecture:component-style-authoring INV6. The overlay goes back to its own copy of the curve, main's literal; it and the row layer's copy are both the constants table's timing.press.highlightOutCurve, and a unit case fails if the overlay's drifts. Red first: a source bundle of Card carried the row layer's constants (cellRelease) on the change before this one; it now carries the overlay and none of them.
The changeset, the release lookup's comment and its unit case named a checkbox's box as a part that draws its release inside the surface. No component in this change does: the checkbox's release runs on the row around it. The lookup still counts a release on a descendant whose nearest press owner is the surface; the text now says that without the example.
…ease change [skip ci]
… change [skip ci]
…get change [skip ci]
There was a problem hiding this comment.
Changes requested: the List touch hit area follows component:List FR10, and its browser spec passes in Chromium at this head with no change for a mouse. This pull request still carries the open #8077, #8890, #8360 and #8076 (29,856 of its 31,863 authored lines), so please land them first and rebase, or split them out.
[Automated review]
Review only this PR's change: vjeux/astryx@9460fdb...ecc8779 (depends on #8077).
This branch also carries one revert commit that takes #8079's divider hiding and #8082's selection hold back out of the line, per Cindy's decisions on both. A build-css test now checks that every press state leaves the dividers drawn; it fails on the head before the revert. Under a finger, a tap in a link row's own padding, outside its anchor, now follows the link too (6527c0f, from review); a mouse click there still does nothing.
User need
A list row is 40 px tall, smaller than the 44 px a finger needs (
spec:AST-094FR42,component:ListFR10). A tap that lands just above or below a row misses it.Problem and solution fit
What led to it: in an iOS table a whole cell takes the tap, so a finger rarely misses. In Astryx the hit area stopped at the row's 40 px box, and taps between rows did nothing.
Under a touch screen, each interactive row's invisible hit area now grows toward 44 px, stopping halfway to the next row or control so no two targets overlap. Nothing moves and nothing paints. A tap in that band on a link row follows the link. A mouse sees no change: a click in a link row's padding still does nothing (
spec:AST-017FR59).Size (gzip, base → head): ListItem JS +2,888 B and CSS +197 B; Item JS +149 B.
Current specification
spec:AST-094FR42 andcomponent:ListFR10, both current.Public delta and caller burden
None. Under a touch screen a row's
::afterholds the hit area, so decorate rows with children; the note inrowPressLayer.tsand its changeset now say so.Design and alternatives
The size is a plain 44 px. No design token defines a larger one yet, so a page's
--size-touch-targetis not read; that arrives with the token (#8241).Evidence
Browser proofs under a finger in Chromium and WebKit (Testing). Rows kept under 44 px by their neighbors: one-line rows reach 42 px (43 px first or last), compact rows with dividers 33 px.
Scope
Testing
Unit and browser tests cover each step in Chromium and WebKit; the new ones failed before the change. Files that time out only when the whole suite runs in parallel pass alone (owned by #8352, #8353, #8471 and #8642).