Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
ccbee43 to
0eab992
Compare
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMultiSelector (@astryxdesign/core) · View in Storybook
Popover (@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/BottomSheet | +15.1KB |
+2B |
| core/ChatLayout | +16.2KB |
+39B |
| core/DateInput | +14.2KB |
+3B |
| core/DateTimeInput | +14.3KB |
+3B |
| core/Popover | +28.5KB |
+4B |
| core/PowerSearch | +14.0KB |
+4B |
| core/TimeInput | +14.3KB |
+4B |
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 | 34.6KB | +26B |
Accessibility Audit
Status: 2 accessibility violation(s) found — 2 serious.
ChatComposer - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 1/19 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
ChatComposerInput - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 1/18 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 | 440,175 |
| Chat | Full AI Chat | probe | dark | 438,105 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Chat | Empty State | probe | light | 122,185 |
| Chat | Empty State | probe | dark | 120,076 |
| 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 |
| 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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request is on hold for an owner decision: no spec record yet says that a Popover presented under a finger should ignore every tap while it animates in, since component:Popover keeps native light dismiss enabled and the draft AST-094 has no rule on input during a transition. An engineering owner can settle it by writing that rule into the record, including whether page pinch-zoom stays available as AST-094 FR10 and FR18 propose, and commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, please land the dependency stack first so this diff is only the hold's own commit (it carries about 33,500 authored lines today), update the useMenuPress record's 500 ms long-press row to the new 150 ms default, and add a WebKit run and a focus check for the held tap.
[Automated review]
| border: '0', | ||
| background: 'transparent', | ||
| overflow: 'hidden', | ||
| 'touch-action': 'none', |
There was a problem hiding this comment.
touch-action: none also blocks page pinch-zoom for the length of the present spring. The draft AST-094 FR10 and FR18 keep pinch-zoom unless a pinch or rotation recognizer is attached; pinch-zoom here would still let no pan start. Worth settling with the owner decision on the hold.
There was a problem hiding this comment.
Done in 943d1b2: the hold's touch-action is now pinch-zoom. No pan starts on it, and a pinch still zooms the page, as draft AST-094 FR10 and FR18 keep it; the unit case asserts it.
| @@ -140,6 +140,7 @@ Consumer migration instructions belong in consumer docs and release notes. | |||
| | stray-click window | 400 ms, or the next `pointerdown` | how long after a tracked release the browser's click is still swallowed | `MENU_PRESS_STRAY_CLICK_MS` | `module:DropdownMenu/useMenuPress` | stable | | |||
| | settle time | 300 ms | proposed: how long an opening press must last before its release may act | `MENU_PRESS_SETTLE_MS` | `module:DropdownMenu/useMenuPress` | proposed | | |||
| | long-press delay | 500 ms | proposed: how long a finger rests on the trigger before the menu opens under it | `longPressDelayMs` | `module:DropdownMenu/useMenuPress` | proposed | | |||
There was a problem hiding this comment.
This row still says 500 ms, but useMenuPress.ts now defaults to menu.holdOpenMs (150 ms) and the hook doc says 150. Update the row with its UIKit source, or keep 500 ms in code until OQ1 is ruled on.
There was a problem hiding this comment.
This record comes from the DropdownMenu press change below this one in the stack, and that change is being brought in line with the code (the 150 ms hold-open and continueGesture). Nothing to change in this pull request.
0eab992 to
882e965
Compare
There was a problem hiding this comment.
This pull request is on hold for an owner decision: no spec record says that a Popover presented under a finger should ignore every touch while it animates in, since component:Popover keeps native light dismiss enabled and the draft AST-094 has no rule on input during a transition (its FR10 and FR18 also keep page pinch-zoom, which the hold's touch-action: none blocks). An engineering owner can settle it by writing that rule into the record and commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, please land the dependency stack first so this diff is only the hold's own commit (it carries about 34,300 authored lines today), update the useMenuPress record's 500 ms long-press row to the new 150 ms default, and add a focus check after the held tap in Chromium and in the iOS Safari run.
[Automated review]
| expect(log.closed).toEqual([]); | ||
| expect(await outsideClicks()).toBe(0); | ||
| // It was the hold that took the tap, inside the popover's own subtree. | ||
| expect(log.held).toEqual([true]); |
There was a problem hiding this comment.
Nothing here checks where focus is after this held tap. The hold is a non-focusable element outside the painted surface, and useFocusTrap leaves focus a pointer moved where it lands; in a fixture that reproduces the hold's DOM, a Chromium touch tap on it moved focus from the Popover's switch to the page body while the Popover stayed open. Assert that document.activeElement is still the Popover's first content control (component:Popover VG1 and AR3), and record the same check in the iOS Safari run.
There was a problem hiding this comment.
The tap case already asserts that the switch the popover focused keeps focus after the held tap. That assertion failed before the hold cancelled the compatibility mousedown and the touch's end. In 943d1b2 the pan case asserts it too. The iOS 26.2 Safari run logged focus on the switch after the held tap.
882e965 to
ded6a6a
Compare
ded6a6a to
943d1b2
Compare
There was a problem hiding this comment.
This pull request is on hold for an owner decision: no spec record says that a Popover presented under a finger should ignore taps and pans while it animates in, since component:Popover keeps native light dismiss enabled and the draft AST-094 has no rule on input during a transition. An engineering owner can settle it by writing that rule into the record and commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, please land the dependency stack first so this diff is only the hold's own commit (it carries about 34,700 authored lines today), and conform the stacked touch presentations to two records that landed on main since: design:reduced-motion DR2 caps a Reduce Motion touch cross-fade at 200 ms (the Popover's fade, and the hold with it, run about 0.53 s), and spec:AST-012 FR9 requires the touch resolver to use the theme's compact definition instead of its own 744 px edge.
[Automated review]
| expect(shown.size).toBe(0); | ||
| }); | ||
|
|
||
| it('holds under Reduce Motion too: the fade runs on the same spring', () => { |
There was a problem hiding this comment.
Under Reduce Motion this case keeps the hold for the whole cross-fade on springs.popoverPresent, which settles in about 532 ms by the animator's own rule. design:reduced-motion DR2, current on main, requires a touch cross-fade to change opacity only and finish in 200 ms or less, so the fade, and the hold that lives for it, should end within 200 ms.
| export const TOUCH_QUERY = '(pointer: coarse)'; | ||
|
|
||
| /** The viewport is compact: layout behaviours. */ | ||
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; |
There was a problem hiding this comment.
spec:AST-012 FR9, current on main, defines compact once (width below the theme's md point, 768, or height below 500) and says the web touch presentation resolver uses that definition and keeps no size numbers of its own. With 744px here, coarse-pointer screens 744 to 767 px wide open the anchored popover where the bottom sheet is required; building the query from the theme's exported compact definition fixes it.
943d1b2 to
98898c3
Compare
github-actions
Bot
added
the
needs:design-review
BottomSheet.test.tsx's imports from both sides are kept; the search goldens are regenerated after the remaining merges.
…'s selection An action group's press is a long press with no minimum duration, so it claims at the press's pointerdown, before any movement. The drag selection hold then cleared the page's selection, so a plain click on a group's button left its Copy or Cut with nothing to act on. The case fails on this head: the selection is gone after the pointerdown.
…age's selection The drag selection hold cleared the page's selection when any mouse or pen contact was claimed, but an action group's press claims at its pointerdown, before it has moved or selected anything, so a plain click on a group's button left its Copy or Cut with nothing to act on. A claim before any movement now keeps the page's selection: the hold still refuses a new one, and puts the page's back over a selection that appears while it runs. A claim after movement clears what its drag selected, as before (spec:AST-094 FR41). The case before the new one now waits for its hold to end a task after the lift, so no hold outlives that test.
…t every pointer does - GestureHost's header, the gestures README and coarsePointer.ts no longer describe the coarse-only host: a finger or a pen takes the touch-down lock under any primary pointer, a kept press's finger is kept during a fine pointer's press too, and recognizers attach for every pointer while the page listeners last a press (spec:AST-094 FR19, FR24, FR41, FR43). - The fine-pointer selection case asserted the replaced rule and passed only because it read iOS WebKit's inline lock without iOS WebKit: it now asserts the lock a fine pointer's finger and pen take, and that a mouse takes none. - FR41's sentence and its verification row say that a press claimed at its own pointerdown, before it moves, keeps the page's selection, while a drag that moved before its claim loses what it selected.
… guide The keyboard layout guide (this pull request) reads where focus moves to tell a keyboard that comes up from one that stays: meta#8559 removed the store's focus reports because nothing there read them, so they return here with their first reader. One pair of capture listeners on the document, attached while anything subscribes, reports each focusin and focusout with the current height; BottomSheet's reveal still acts on viewport reports only.
Conflicts were in this stack's own keyboard files: the browser-support sentence keeps ChatLayout's composer and takes meta#8559's page wording, BottomSheet's test imports follow the sheetKeyboard split, and the keyboard notes keep both the sheet's and ChatLayout's Chrome 124 findings. The search goldens are regenerated after the merge.
…op-level page The composer's tap route, its scroll-free focus and Done's park attach only where the keyboard is measurable, which excludes a page inside an iframe (spec:AST-099 FR7); there the browser's own focus applies. The docs and changeset now say so.
…ion, in the present tense The sentence said a drag that moved before its claim loses the selection it started, but the hold clears the page's selection then, whatever made it; and an unclaimed mouse drag kept "today's" behavior, a phrase that ages. It now says a claim after movement clears the page's selection, a press claimed at its own pointerdown keeps it, and an unclaimed mouse drag keeps its released behavior.
This pull request now sits on meta#8665, so a stack carrying it and meta#8665's other dependants (meta#8676) needs no second GestureHost resolution. Two hunks in GestureHost.ts's pointerdown, both this pull request's: - the second-finger check where fingers come from pointer events stays, followed by meta#8665's rule that a right click, a pen's barrel button or a macOS ctrl-click starts no press; - the check that runs again once the new finger is delivered stays, followed by meta#8665's selection lock call, which no longer takes the window.
…e Popover touch presentation AST-057 takes the dependencies' text, which is main's (meta#8646): the tuning bullet this branch once restored is superseded, and the Popover touch presentation does not touch row actions.
…e Popover presentation hold
… ChatLayout BottomSheet's useSheetKeyboard moved to keyboard/sheetKeyboard.ts, and it subscribes to the keyboard layout guide and rides Gboard with rideKeyboard as ChatLayout does, but the two modules' headers named only ChatLayout, and BottomSheetPanel's related-files list and the keyboard notes still pointed at keyboardHeight.ts. They now name the hook and its file.
…the Popover touch presentation
Once a component in the root entry holds its layer's input (a popover's presentation hold, meta#8223), Layer/inputHold's static imports of gestures/GestureHost and gestures/nativeScroll put eight gesture modules (about 47 KB) in the root entry, against architecture:react-component-runtime INV13. Bundle a component that imports the input hold and require no gesture bytes. Fails while the input hold imports from gestures/.
…e host asks it Layer/inputHold registered a touch gate with the gesture host (getGestureHost(doc).addTouchGate(isInHeldLayer)) and walked the flat tree through gestures/nativeScroll, so any component that holds its layer statically carried the gesture system into its bundle and the root entry (architecture:react-component-runtime INV13). Invert it: the host asks isInHeldLayer for every touch itself (gestures may import Layer), the hold registers nothing, and it walks the flat tree through utils/flatTree, where parentAcrossShadow already lives. addTouchGate stays for other gates. A host test holds a held layer's touches with no gate (it fails if the host stops asking the hold).
…e input hold imports nothing from gestures/
…ut hold imports nothing from gestures/) into the Popover presentation hold
There was a problem hiding this comment.
Changes requested: the Popover presentation hold itself follows spec:AST-094 FR35 and component:Popover FR9, but this diff against main still carries its whole dependency stack, 44,411 authored lines against the hold's own 452 in four files. Please land the dependencies (#8587, #8187 and #8342, with the pull requests they merge) and rebase, so this diff holds only the hold's four files and the record text it inherits (architecture:layer-runtime INV13 with its Exit copy section, and the new spec:AST-094 FR41 selection rule) lands through its own approved spec pull requests.
spec:AST-057now says the row's slide and spring play on theme motion roles, butuseSwipeAction.tsstill settles on a fixed 200 ms; please restoremain's text where it comes from (#8186).- The FR35 browser cases cast no storyboard, which the
spec:AST-094FR34/FR35 verification row asks for.
[Automated review]
| correct, and the runtime polls nothing: an anchor that a scrolled | ||
| container hides is followed again when an intersection observer reports it | ||
| back in view. | ||
| - **INV13 — One exit copy per closing layer.** A layer's close plays on at most |
There was a problem hiding this comment.
This INV13, with the Exit copy section (lines 304-321) and the rows at 565-571, 602-607, 629-632 and 682, is record text main does not have, and an engineering owner asked that record text land only through an approved spec pull request. It comes from #8186: land it there, or in its own spec pull request, and rebase so this diff no longer carries it.
| acts as a finger's; a mouse makes the context request by a right click, not | ||
| a long press (FR43), and otherwise keeps its released behavior | ||
| (`spec:AST-017` FR59; DEC-9). | ||
| (`spec:AST-017` FR59; DEC-9). A mouse or pen drag that a recognizer claims |
There was a problem hiding this comment.
This makes a claimed mouse or pen drag take no new selection and clear the page's selection, while current FR41 says a mouse “otherwise keeps its released behavior” and its verification row fails any change to mouse selection. It arrives from #8336 through #8342, so it needs its own approved spec change rather than riding this Popover fix.
| - **Gesture tuning.** Axis-lock distance, commit ratio, fling velocity, | ||
| resistance past the panel: behavior constants in source, not public API and | ||
| not design tokens (the same ruling the touch press model received for its | ||
| clocks). The row's slide and spring motion plays on theme motion roles |
There was a problem hiding this comment.
The row still settles on the literal SWIPE_SETTLE_MS = 200 (packages/core/src/Item/useSwipeAction.ts:86, :368), so this sentence describes motion the row does not have. An earlier commit on this branch removed it for that reason and a merge of #8186's line restored it; please restore main's text here and at line 223, or move the row onto a role in the same change.
| expect(log.closed).toEqual([]); | ||
| }); | ||
|
|
||
| test('a tap or a drag that lands while it grows closes nothing, moves no focus and scrolls nothing (spec:AST-094 FR35)', async ({ |
There was a problem hiding this comment.
The spec:AST-094 FR34/FR35 verification row asks for “Browser tests and storyboards under a finger”, and none of the FR35 cases casts a storyboard. A castStoryboard of the held tap, as the busy-thread case does, would complete it.
Review only this PR's change: vjeux/astryx@0181da7...3f21fd4
Depends on #8587 (one presentation hold), #8187 and #8342; 0181da7 merges them. #8662 stacks on this.
An open review asks to land the dependencies and rebase, so this diff holds only the hold's four files; the hold described here does not change.
This pull request shows a merge conflict with main in the CLI's icon-set search goldens; it comes from the keyboard module below it (#8559), which resolves it, and this branch takes that fix with its next push.
Why
With a finger, a tap that landed while a Popover was still growing out of its trigger closed it again, and a drag scrolled the page and dismissed it. A person who taps twice quickly, or starts scrolling a moment too early, loses the popover they just opened. On iOS both are ignored: the presentation finishes and the popover stays.
What led to it
We are bringing Astryx's touch components to the behavior of native iOS controls. On the iOS simulator, UIKit ignores every touch while a popover presents;
spec:AST-094FR35 asks the same of ours.Options and tradeoffs
preventDefaulton capture listeners still closes it in Chromium.Why this choice
It is the only option that also stops the browser's own light dismiss and scrolling, and it reuses the shared hold: no API of its own and no second hold. Not built, as an owner's call: UIKit also swallows the tap that dismisses.
How to check it
Evidence
iOS 26.2 simulator (Safari), a second tap about 170 ms in, then a drag up from outside 100–130 ms in:
Android 15 (Chromium), a tap about 210 ms in: