Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
Modified ComponentsAppShell (@astryxdesign/core)
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/AppShell | +22.7KB |
+334B |
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 | 35.4KB | 35.8KB | +415B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 504 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 | 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 needs an engineering owner's decision before it can be approved: no current spec covers TopNav's touch phone bar, its glass capsule or the new --color-background-glass token, and the draft touch-presentations spec it carries is not approved. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately, the @astryxdesign/cli docs and theme template changes need a Changeset naming that package (the lint check fails), and a phone bar with no end items shrinks to 42px with its title against the top edge instead of the documented 54px bar. Landing the stacked predecessors first would also let this pull request show only its own commit. [Automated review]
| paddingBlockStart: { | ||
| default: spacingVars['--spacing-2'], | ||
| '@media (pointer: coarse)': 0, | ||
| }, | ||
| paddingBlockEnd: { | ||
| default: spacingVars['--spacing-2'], | ||
| '@media (pointer: coarse)': '10px', | ||
| }, |
There was a problem hiding this comment.
On a coarse pointer these two paddings add up to 54px only when the 44px capsule renders. A phone bar with no endContent and no drawer content measures 42px with its heading against the top edge (48px and centred on the base), while the Changeset and TopNav.doc.mjs promise a 54px bar. A 54px minimum block size under (pointer: coarse) with the items on a 44px row would keep the geometry; the empty-end-group test could assert it.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@astryxdesign/core': patch | |||
There was a problem hiding this comment.
The glass token also changes shipped @astryxdesign/cli files (packages/cli/assets/docs/tree/tokens-color.doc.mjs, tokens.doc.mjs, packages/cli/assets/theme.template.ts), and the lint job's Changeset coverage step fails because no Changeset names that package. Adding '@astryxdesign/cli' here would clear it; the text could also name the old touch bar (48px, 8px padding, bare end icons) and the token override as the way to restyle the capsule.
| const {container} = renderPhoneBar(profile()); | ||
| const capsule = container.querySelector('[data-bar-capsule]')!; | ||
| expect(capsule).not.toBeNull(); | ||
| expect(capsule.contains(screen.getByRole('button', {name: 'Profile'}))); |
There was a problem hiding this comment.
This expect(...) has no matcher, so it never checks that the Profile button is inside the capsule; .toBe(true) would make it assert.
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation. The head quoted here predates the change; the next push replaces TopNav's own |
7bd8556 to
94fed56
Compare
vjeux
changed the title
|
Pushed 94fed56: the bar and its capsule now key on a coarse pointer, one touch model with Android included (#8316), with no iOS gate, and a mouse keeps today's bar. The key stays the literal |
github-actions
Bot
added
the
needs:spec-owner-review
|
Converting to draft: the capsule's glass material waits on the look-and-feel decision (the 03:41 note on #8283). I'll bring it back when that decision lands. |
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved: no current spec covers TopNav's touch phone bar or its glass capsule, and the new --color-background-glass token relies on this branch's amendment to the theme-token grammar (architecture:theme-tokens INV10), which is not approved yet. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision, please:
- regenerate and commit the native Swift theme (
pnpm -F @astryxdesign/native generate:swift-ios), whose drift test fails in CItest-node; - keep the touch bar 54px when it has no end items (a heading-only bar is 42px with its title against the top) and draw no capsule when the end content renders nothing;
- name the old touch bar and
@astryxdesign/cliin the Changeset, add the base-against-head component size report (AppShell now imports the animator and gesture host for a touch-only effect), and add the missing matcher atTopNavBarCapsule.test.tsx:136.
Landing the stacked predecessors first would also let this pull request show only its own commit. [Automated review]
| }, | ||
| paddingBlockStart: { | ||
| default: spacingVars['--spacing-2'], | ||
| '@media (pointer: coarse)': 0, |
There was a problem hiding this comment.
Under a coarse pointer the bar has 0 top and 10px bottom padding, which makes 54px only when the 44px capsule renders. With no endContent and no drawer content the bar measures 42px with the heading flush against its top edge (48px and centred on the base), while the Changeset and TopNav.doc.mjs promise a 54px bar. A minBlockSize: '54px' under the same key, with the items on a 44px top row, would keep the geometry; the empty-end-group test could assert it.
| const showToggle = hasMobileDrawerContent && hasAutoToggle; | ||
| // An empty end group draws no capsule. | ||
| const hasEndItems = | ||
| (endContent != null && endContent !== false) || showToggle; |
There was a problem hiding this comment.
This reads the endContent value, not what renders: endContent={<>{signedIn && <Button … />}</>} with signedIn false, or a component that returns null, draws a 12 × 44 glass capsule with no children. Hiding the capsule when it is empty (display: {default: 'flex', ':empty': 'none'} on mobileBarEndCapsule) would cover every case.
| // platter: the theme's surface at 85 %, so content scrolling under the bar | ||
| // still shows faintly through it. iOS 26.2 paints it rgb(251, 251, 255) over | ||
| // its light bar and rgb(25, 25, 25) over its dark one; no blur. | ||
| '--color-background-glass': |
There was a problem hiding this comment.
The new token also has to reach the committed native Swift theme: swiftIOSTheme.test.ts (drift G13) fails in CI test-node (1/4) with “Tokens.swift is stale; run pnpm -F @astryxdesign/native generate:swift-ios”. Running that command and committing the regenerated files clears it.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@astryxdesign/core': patch | |||
There was a problem hiding this comment.
This commit also changes shipped @astryxdesign/cli files (tokens-color.doc.mjs, tokens.doc.mjs, theme.template.ts), so '@astryxdesign/cli': patch belongs here; today the lint gate passes only through a predecessor's Changeset. The text could also name the old touch bar (48px, 8px padding, bare end icons) and the token as the way to restyle the capsule, as AST-017 FR3 asks of an appearance change.
| const {container} = renderPhoneBar(profile()); | ||
| const capsule = container.querySelector('[data-bar-capsule]')!; | ||
| expect(capsule).not.toBeNull(); | ||
| expect(capsule.contains(screen.getByRole('button', {name: 'Profile'}))); |
There was a problem hiding this comment.
This expect(...) has no matcher, so it never checks that the Profile button is inside the capsule; .toBe(true) makes it assert.
|
Posted by Cindy's agent on her behalf. Thanks for this! The same guardrails Cindy set on #8142 apply here. Could you propose an approach that fits them?
The element count already complies: this commit adds no elements. |
|
Thanks. Here is the same approach as on #8142, applied to the phone bar. Height and insets, through the existing target. The touch geometry moves onto the Capsule fill, with no new token. Theming spec. No new target, prop or attribute. The capsule is TopNav's existing Flex area (end content and the drawer toggle), painted in one mode. The guaranteed paddings get compiler and runtime evidence on Variants. Only If this works for you, I'll implement it and post evidence. |
94fed56 to
b4c395b
Compare
…opNav phone-bar capsule
…xperimental touch exports The motion README's Files table is the union: main's rows, with meta#8663's resolveSnapPoint.ts and selectPage.ts, plus this branch's, in main's order. # Conflicts: # packages/core/src/motion/README.md
…on/README.md) into facebook#8309
…the scroll edge effect's bases
…opNav phone-bar capsule
…ntries and internal/motion Main's resolveSnapPoint test (meta#8663) keeps the release policy internal: not on the package root, not in a barrel, and no package export naming motion. The last check also caught ./internal/motion, Core's internal entry for Astryx's own packages (architecture:react-component-runtime INV13), which re-exports no release policy. The export check now covers the public keys only, and a new assertion keeps the claim as strong: internal/motion must not re-export resolveSnapPoint either. On this branch with main the test failed on ./internal/motion; it now passes, and it fails when internal/motion re-exports resolveSnapPoint.
…fix) into the scroll edge effect's bases
…opNav phone-bar capsule
… a mouse press At the document, an event from inside an open shadow root is retargeted to the shadow host, and so is a touch's target, so the host looked for participants from the shadow host and found none inside the root: the browser then sent its own mousedown and click. Both cases, a finger and a mouse, attach a tap to an element in the shadow root. Fail until the host resolves targets through the composed path.
…composed path Inside an open shadow root the document's listeners see the shadow host, in event.target and in a touch's target alike, so a recognizer on an element in the root never got its finger, and nothing claimed it. landedOn takes the composed path's first element for the event's own target and for a touch whose target is the event's (another finger keeps its own): a touch's start, a pointer's press and its pairing, the last touch a scroll reads, a cancelled pointer's touch and hover.
…he host resolves targets through the composed path) into meta#8320
…s its divider with a coarse-pointer rule, and drops its layer where no scrollend fires Review findings on meta#8142: - With a mouse, a variant added by module augmentation painted a surface header. A page-scrolling shell now keeps each variant's own fill (none for an augmented one) and adds only the coarse-pointer transparent rule. - The section divider was dropped through the resolver's guess, so a server render with `defaultIsMobile` lost it. It stays in the markup, and a coarse-pointer rule takes its width away on the touch bar. - Where the browser sends no `scrollend`, the layer stayed promoted after a fling (re-checked only at a finger change). The scroller's scroll events now time a re-check 200 ms after the last one, past the finger-scroll watch's 150 ms spell. - One document listener per effect: `scrollend`, or `scroll` where the browser sends none. This base has no shared input stream (it arrives with meta#8665's line); the effect moves onto it there. - The WebKit workaround names its removal condition. Red first: the five new cases fail on ae06884.
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved: no current spec covers TopNav's touch capsule, so an engineering owner needs to approve or redirect it, or name the record that governs it, by commenting /approve-spec <full head SHA>; the author may do so as an engineering owner. Independently of that decision, please:
- keep the default look's sizes under touch, as
spec:AST-094FR42 and DEC-20 require, since the 54px bar, 16px insets and 44px end group belong in a theme adaptation unless those claims are amended; - keep the touch bar's height and a centred heading when it has no end items, and draw no capsule when the end content renders nothing;
- add the missing matcher at
TopNavBarCapsule.test.tsx:138and name the old touch bar in the Changeset.
scripts/touch-support-bundle.test.mjs also fails on this head because of the AppShell change from #8142, and landing the stacked predecessors first would let this pull request show only its own change. [Automated review]
| padding: spacingVars['--spacing-2'], | ||
| paddingInline: { | ||
| default: spacingVars['--spacing-2'], | ||
| '@media (pointer: coarse)': spacingVars['--spacing-4'], |
There was a problem hiding this comment.
Under a coarse pointer this widens the insets from 8px to 16px, and the block paddings below grow the bar from 48px to 54px. spec:AST-094 FR42 says the default look makes no box size increase for phones or touch and that mobile size increases belong to theme adaptations (DEC-20). Keeping the default boxes and moving this geometry into a theme adaptation (when: {pointer: 'coarse'}) would conform, unless an engineering owner amends FR42/DEC-20.
| }, | ||
| paddingBlockEnd: { | ||
| default: spacingVars['--spacing-2'], | ||
| '@media (pointer: coarse)': '10px', |
There was a problem hiding this comment.
With 0 above and 10px below, the bar is 54px only while the 44px capsule renders. A heading-only phone bar (no endContent, no drawer content) measures 42px under a finger with its heading against the top edge, where the base gives 48px with the heading centred. A minimum block size of the capsule's height plus 10px under the same key would keep the geometry, and the empty-end-group test could assert it.
| const showToggle = hasMobileDrawerContent && hasAutoToggle; | ||
| // An empty end group draws no capsule. | ||
| const hasEndItems = | ||
| (endContent != null && endContent !== false) || showToggle; |
There was a problem hiding this comment.
This reads the endContent value, not what renders: endContent={<>{signedIn && <Button … />}</>} with signedIn false, or a component that returns null, draws a 12 × 44 capsule with no children. display: {default: 'flex', ':empty': 'none'} on mobileBarEndCapsule would cover every case.
| const {container} = renderPhoneBar(profile()); | ||
| const capsule = container.querySelector('[data-bar-capsule]')!; | ||
| expect(capsule).not.toBeNull(); | ||
| expect(capsule.contains(screen.getByRole('button', {name: 'Profile'}))); |
There was a problem hiding this comment.
This expect(...) has no matcher, so it never checks that the Profile button is inside the capsule; .toBe(true) would make it assert.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] TopNav's phone bar under a finger (one touch model on every touch device, iOS and Android alike) takes the iOS 26 bar's geometry, and its end items sit together on one capsule. The geometry comes from the theme's spacing and size tokens: with the defaults the bar is 54px tall, with its items on the top 44px and 16px from the screen's sides, and the capsule is 44px tall and fully rounded, placing each control where UIKit centres its 36px cells. The capsule paints the theme's `--color-background-surface`, with no shadow and nothing blurred. No token is added. With a mouse the bar is unchanged. |
There was a problem hiding this comment.
spec:AST-017 FR3 asks an appearance Changeset to name the old and the new look. This one describes the new touch bar only; naming the old one (48px, 8px insets, bare end icons) would complete it.
Resolves the conflict in the search-icon goldens: main and this branch both moved the "icon" match count. Regenerated with `vitest run packages/cli/test/experimental-isolation.test.mjs --update` on the merged tree; the goldens differ from main's only by this branch's TopNav docs line (224 matches for main's 223).
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: the touch phone bar's 54px height and 44px end group contradict spec:AST-094 FR42 and DEC-20, and no current spec governs the surface capsule, so an owner needs to approve the capsule and either amend FR42/DEC-20 on main or move the growth into a theme adaptation, recording that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do this as an engineering owner). Whatever that decision is, these changes are still needed:
- Split out the carried #8142/#8562 stack, which makes up 27,773 of the 28,319 authored lines and includes the failing root-entry test in
scripts/touch-support-bundle.test.mjs. - Fix the 42px heading-only bar, the empty capsule drawn when end content renders nothing, the missing matcher at
TopNavBarCapsule.test.tsx:138, and the Changeset's missing old look.
[Automated review]
Stacked on #8142 (the scroll edge effect). Review only this PR's change: vjeux/astryx@4fe659b...8ba07f7
Carries #8562 (Core's internal entries) as a dependency; if the entries land differently, this follows her decision.
User impact
Under a finger (one touch model on every touch device;
spec:AST-094FR13), TopNav's end icons float bare over content scrolling under the bar; iOS 26 puts bar buttons on capsules. A mouse keeps today's bar.Visual authority
motion/uikit/bars.ts) is built from the spacing and size tokens; per chore(core): the touch motion tables register FR4's roles as internal motion roles #8257 (spec:AST-094DEC-11), nothing new is public.Owner decisions (proposed; pending owner review):
@media (pointer: coarse)key) the phone bar is 54px tall with the default tokens (iOS 26.2) instead of 48, its items--spacing-4from the sides.--color-background-surface, opaque, with no blur or shadow. iOS's content-following colour is not modelled: over a surface-coloured page it shows only over content.Before and after
With the default tokens, the bar is 54px with its items on the top 44px. The end items share one fully rounded capsule: a
--size-element-lgcell plus--spacing-1above and below (44px), each control where UIKit centres its cells (--spacing-1in,--spacing-4apart).iOS 26.2 Safari, #8142 (left) and this branch (right), scrolled (top) and at rest (bottom):
Dark mode (P4) and right to left (P5), #8142 (top) and this branch (bottom):
Android 15, Chromium 158 (emulator), main (left) and this branch (right):
State matrix
Size (INV6,
analyze-pr, gzip, base → head): TopNav's import 53,375 → 53,507 B of JavaScript; its CSS 10,610 → 10,779 B.Scope
Testing
pnpm exec vitest run packages/core/src/{TopNav,motion,AppShell,theme,hooks,internal}: 95 files, 3006 tests pass.TopNavBarCapsule.test.tsxreads the raw StyleX rules: the bar and capsule declarations (token expressions) under the coarse pointer alone, none gated on WebKit for iOS; the mouse path, first asserting a fine pointer; the empty end group; and the default tokens give the geometry measured inbars.ts.pnpm test:interaction TopNavBarCapsule AppShellScrollEdgeagainst a freshpnpm storybook:build: 19 pass in Chromium; 15 pass and 4 Chromium-only skips in WebKit (official Playwright image). Bar height, capsule geometry and radius; its colour equals the theme's surface (a probe beside it); no shadow; button centres 22/74; names and order; a real touch on the toggle opens the drawer; right to left (the capsule 16px from the left); Reduce Transparency changes nothing (Chromium); a mouse keeps today's bar; a still.rgb(255, 255, 255)(the light surface), no shadow, buttons centred at 22/74.scripts/touch-support-bundle.test.mjs› "is absent from the root entry" is red: AppShell's touch bar imports the gesture host statically, as INV13 prescribes, so the test's whole-root claim can't hold; F1's per-component bundle test, test(core): a touch component's import carries its gesture code, and nothing else does #8671, lists AppShell as a gesture user.pnpm a11y:audit -- --components TopNav,AppShell: no new violations.pnpm exec vitest run --project node packages/cli/test: 309 pass (thesearch icongolden counts the new dense-docs line: 220 → 221 matches). Rootpnpm typecheckafterpnpm build(CI's order),check:changesets,check:knowledge, the CI duration guard,internalModulesNotExperimental,scripts/lab-core-imports.test.mjs, eslint, prettier and the pre-commit checks pass.