Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 is on hold until an engineering owner decides the new public AppShell contract this adds: height="adaptive", the root data-scroll-owner attribute, and mobileNav.defaultIsMobile seeding the touch query on the server. No current spec covers any of them yet. An engineering owner can record that decision by commenting /approve-spec 72d78d90d603c7b549dbf3f7b76c794183b07586 on this pull request or by identifying the record that should hold it, and the author may do so if they are an engineering owner. Separately from that decision, a few fixes are needed:
- The new
AdaptiveHeightstory fails thepr-a11ycheck (scrollable-region-focusableon the main region in its mouse,fill, state); make the overflowing main keyboard-reachable or get a reviewer's blessing for a baseline entry. defaultIsMobileis still documented as ignored whenbreakpointisnone, but it now seedsadaptive's server render in that case too.- The AppShell component description still says it supports two height modes.
[Automated review]
| // server-rendered phone page starts out scrolling the page. | ||
| const isTouchPointer = useMediaQuery( | ||
| TOUCH_POINTER_QUERY, | ||
| mobileNavConfig?.defaultIsMobile ?? false, |
There was a problem hiding this comment.
defaultIsMobile now also seeds the touch query, including under breakpoint: 'none', where the prop's JSDoc (lines 172-175) and AppShell.doc.mjs say it is ignored. A server render with {breakpoint: 'none', defaultIsMobile: true} gives data-scroll-owner="document", and a fine-pointer client switches to content after hydration. Either document this second role (JSDoc plus the three doc entries) or gate it like the breakpoint seed at line 554.
There was a problem hiding this comment.
Fixed in 7906356 by documenting the second role: the prop JSDoc and the three doc entries now say that with height="adaptive", defaultIsMobile also seeds the server render's touch assumption whatever the breakpoint, while breakpoint none still ignores it for the layout. The component summary now lists three height modes.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsAppShell (@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. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether height="adaptive", the root data-scroll-owner attribute, and mobileNav.defaultIsMobile seeding the touch query on the server become AppShell's public contract, since no current spec covers them yet. An engineering owner can record that decision by commenting /approve-spec 53b41e8e713f4a80409eaf90937c00d2a59df1fa on this pull request or by identifying the record that should hold it, and the author may do so if they are an engineering owner. The keyboard stop added to the Adaptive Height story resolves the earlier accessibility failure; two documentation fixes are still needed:
defaultIsMobileis still documented as ignored whenbreakpointisnone, but it now seeds the server render ofadaptivein that case too.- The AppShell component description still says it supports two height modes.
[Automated review]
…leaner transition fill
|
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. |
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether height="adaptive", the root data-scroll-owner attribute, and mobileNav.defaultIsMobile seeding the touch query on the server become AppShell's public contract, since no current spec covers them yet. An engineering owner can record that decision by commenting /approve-spec 7906356a046c22de4e9b870c36235647a728ca86 on this pull request or by identifying the record that should hold it, and the author may do so if they are an engineering owner. Separately, one fix is needed: when the primary pointer changes at runtime, adaptive remounts the side navigation, so a sidebar the user collapsed expands again, because AppShell.tsx:801-806 wraps the side nav in a sticky div only in auto mode; keeping that element tree the same in both modes, with a test that a collapsed SideNav survives the switch, would fix it. The earlier documentation points are resolved on this head.
[Automated review]
| const scrollsDocument = | ||
| height === 'auto' || (height === 'adaptive' && isTouchPointer); | ||
| const isFill = !scrollsDocument; | ||
| const isAuto = scrollsDocument; |
There was a problem hiding this comment.
When isTouchPointer changes under height="adaptive", isAuto flips and sideNavContent (lines 801-806) switches between the bare LayoutPanel and the sticky wrapper div, so React remounts the side navigation: a <SideNav collapsible> the user collapsed expands again after the pointer turns coarse, while main keeps its state. Keeping the side-nav element tree the same in both modes (render the wrapper in both and apply the sticky styles only for auto), and asserting that in the "adaptive follows the primary pointer when it changes" test, would cover it.
|
Agreed: one implementation.
|
7906356 to
e1349f7
Compare
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether component:AppShell should be amended to admit height="adaptive" (keyed on the experimental touch resolver), mobileNav.defaultIsMobile seeding it on the server under every breakpoint, and the root data-scroll-owner attribute, since the current AppShell record allows only fill and auto; an engineering owner can record that decision by commenting /approve-spec e1349f7d94d0d532e5d5913d5d1a35cc95390f36 on this pull request or by identifying the record that should hold it, and the author may do so as an engineering owner. Independently of that decision, these need changes:
- This branch now carries the whole #8076 stack (about 29,100 of its 29,300 changed lines); rebase it onto
mainonce that stack lands so only the five AppShell files remain. - A runtime pointer change under
adaptiveremounts the side navigation, so a collapsedSideNavexpands again (AppShell.tsxlines 796-801); keep the same element tree in both modes and extend the pointer-change test. - Any other
heightvalue now behaves asfillinstead of the recordedautominimum height without sticky regions (AppShell.tsxline 615). - The carried theme
platformadaptation condition goes beyond the closed condition vocabulary ofspec:AST-012FR2 and needs an amendment or removal in the stack's own pull request.
[Automated review]
| }); | ||
| const scrollsDocument = | ||
| height === 'auto' || (height === 'adaptive' && isTouch); | ||
| const isFill = !scrollsDocument; |
There was a problem hiding this comment.
With isFill = !scrollsDocument, any value outside fill, auto and adaptive now gets fill (root height: 100dvh, main overflow: auto), while the base gave it the auto minimum height without sticky regions, as component:AppShell records. Resolving only adaptive keeps that:
const resolvedHeight = height === 'adaptive' ? (isTouch ? 'auto' : 'fill') : height;
const isFill = resolvedHeight === 'fill';
const isAuto = resolvedHeight === 'auto';| const scrollsDocument = | ||
| height === 'auto' || (height === 'adaptive' && isTouch); | ||
| const isFill = !scrollsDocument; | ||
| const isAuto = scrollsDocument; |
There was a problem hiding this comment.
Still open on this head: when isTouch changes under height="adaptive", isAuto flips and sideNavContent switches between the bare LayoutPanel and the sticky wrapper div, so React remounts the side navigation and a collapsed <SideNav collapsible> expands again (reproduced in Chromium and jsdom). Rendering the wrapper in both modes with the sticky styles only for auto, and asserting collapsed state in "adaptive follows the primary pointer when it changes", would cover it.
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether component:AppShell should be amended to admit height="adaptive" (keyed on the experimental touch resolver), mobileNav.defaultIsMobile seeding it on the server under every breakpoint, and the root data-scroll-owner attribute, since the current AppShell record allows only fill and auto. An engineering owner can record that decision by commenting /approve-spec efa5550100bbbf0a2da4bf40949382918faf5706 on this pull request or by identifying the record that should hold it, and the author may do so as an engineering owner. Independently of that decision, these need changes:
- This branch carries the whole #8076 stack (about 29,100 of its 29,350 changed lines); rebase it onto
mainonce that stack lands so only the five AppShell files remain. - A runtime pointer change under
adaptiveremounts the side navigation, so a collapsedSideNavexpands again (AppShell.tsxlines 796-801); keep the same element tree in both modes and extend the pointer-change test. - Any other
heightvalue now behaves asfillinstead of the recordedautominimum height without sticky regions (AppShell.tsxline 615). - The carried theme
platformadaptation condition goes beyond the closed condition vocabulary ofspec:AST-012FR2 and needs an amendment or removal in the stack's own pull request.
[Automated review]
efa5550 to
b4e1d3a
Compare
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether component:AppShell, which today allows only fill and auto, should be updated to admit height="adaptive" (keyed on the experimental touch resolver), mobileNav.defaultIsMobile seeding it on the server under every breakpoint, and the root data-scroll-owner attribute. An engineering owner can record that decision by commenting /approve-spec 5300d0a42c2a00fa50e5a7fb6503e5f8a480de93 on this pull request or by updating the record, and the author may do so as an engineering owner. Whatever the decision, the branch needs a rebase, and if adaptive stays it needs two fixes:
- This branch carries the whole #8076 stack (about 30,600 of its 30,800 authored lines); rebase it onto
mainonce that stack lands so only the five AppShell files remain. - A runtime pointer change under
adaptiveremounts the side navigation, so a collapsedSideNavexpands again (AppShell.tsxlines 796-801); keep the same element tree in both modes and extend the pointer-change test. - Any other
heightvalue now behaves asfillinstead of the recordedautominimum height without sticky regions (AppShell.tsxline 615).
[Automated review]
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether component:AppShell, which today allows only fill and auto, should be updated to admit height="adaptive", mobileNav.defaultIsMobile seeding it on the server under every breakpoint, and the root data-scroll-owner attribute; an engineering owner records that decision by commenting /approve-spec a4844b1ce56f98b7bfdea1afb79cb230bb1398f6 on this pull request (the author may do so as an engineering owner), and whatever the decision the branch needs a rebase, plus two fixes if adaptive stays:
- Rebase onto
mainonce #8076 lands, so only the five AppShell files remain. - Keep the side navigation mounted when the primary pointer changes under
adaptive, since today a collapsedSideNavexpands again (AppShell.tsxlines 796-801). - Give a
heightoutside the union the recordedautominimum height instead offill(AppShell.tsxlines 613-616), unless the record update settles it.
[Automated review]
| }); | ||
| const scrollsDocument = | ||
| height === 'auto' || (height === 'adaptive' && isTouch); | ||
| const isFill = !scrollsDocument; |
There was a problem hiding this comment.
With isFill = !scrollsDocument, a height outside fill, auto and adaptive now renders the fill frame (root height: 100dvh, main region scrolling); the base and component:AppShell line 142 give it the auto minimum height without sticky regions. Computing isFill as height === 'fill' || (height === 'adaptive' && !isTouch) and deriving data-scroll-owner from it keeps the recorded behavior.
There was a problem hiding this comment.
Fixed in 64ba8ef: isFill is now height === 'fill' || (height === 'adaptive' && !isTouch), so a height outside the union keeps the recorded auto minimum height with no sticky regions, and data-scroll-owner follows it. A unit test pins it.
| const scrollsDocument = | ||
| height === 'auto' || (height === 'adaptive' && isTouch); | ||
| const isFill = !scrollsDocument; | ||
| const isAuto = scrollsDocument; |
There was a problem hiding this comment.
Under adaptive, isAuto now flips with the primary pointer, and sideNavContent (lines 796-801) wraps the side panel in a sticky div only while it is set, so every pointer change unmounts and remounts the side region and a collapsed SideNav expands again. Keeping the wrapper in both modes and toggling only its sticky style keeps it mounted; the pointer-change test could assert that a collapsed SideNav stays collapsed.
There was a problem hiding this comment.
Fixed in 64ba8ef: under adaptive the side panel's wrapper stays in both pointer states and only its style changes (no box with a fine pointer), so a pointer change no longer remounts the side region. The pointer-change test collapses a SideNav and checks it stays collapsed across both changes.
…nd keeps independent regions with a mouse
…inside its scrolling main region
…ve's server render The component summary still listed two height modes. defaultIsMobile was documented as ignored under breakpoint 'none', but with height="adaptive" it also seeds the server render's touch assumption, whatever the breakpoint; the JSDoc and the three doc entries now say so.
… touch resolver AppShell kept its own (pointer: coarse) query for height="adaptive". It now reads isTouch from useTouchPresentation, the one definition of when a component presents itself the touch way (AST-012 FR2), seeded on the server by mobileNav.defaultIsMobile as before. A source test keeps AppShell on the resolver; the changeset says "under a finger".
…eries, not StyleX keys
Touch is read in script only through useTouchPresentation: the check
rejects a media query on the pointer in matchMedia or useMediaQuery,
literal or through a constant. A StyleX key spelled as the resolver's
constant ('@media (pointer: coarse)') stays allowed, since the
resolver's spelling test pins it.
…r change, and an unknown height stays auto Under height="adaptive" the side panel's sticky wrapper came and went with the primary pointer, so every pointer change remounted the side region and a collapsed SideNav expanded again: the wrapper now stays in both pointer states and only its style changes (no box with a fine pointer). And a height outside the union rendered the fill frame; it keeps the recorded auto minimum height with no sticky regions again, and data-scroll-owner follows.
a4844b1 to
64ba8ef
Compare
|
An engineering owner's spec approval on this pull request's head records the decision. |
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether component:AppShell should be updated to admit height="adaptive", the root data-scroll-owner attribute (which now replaces a caller's own value), and mobileNav.defaultIsMobile seeding adaptive's server render under every breakpoint, a seed that also departs from spec:AST-094 FR13, where the server and hydrating renders produce the pointer presentation. An engineering owner records that decision by commenting /approve-spec 64ba8ef17da62c06585e7dfa52f90d4f036b2757 on this pull request, and the author may do so as an engineering owner. If the seed stays, it also needs the real-browser server-render and hydration evidence that spec:AST-013 FR10 requires; the earlier side-navigation remount and out-of-range height findings are resolved on this head.
[Automated review]
| // gets none of them. The SSR hint for mobile layout also seeds it, so a | ||
| // server-rendered phone page starts out scrolling the page. | ||
| const {isTouch} = useTouchPresentation({ | ||
| isTouch: mobileNavConfig?.defaultIsMobile ?? false, |
There was a problem hiding this comment.
This seed makes adaptive's server and hydrating render the touch layout whenever defaultIsMobile is set, including under breakpoint: 'none'. spec:AST-094 FR13 has both renders produce the pointer presentation and component:AppShell FR8 ignores the hint under none, so the seed is part of the owner decision; if it stays, spec:AST-013 FR10 asks for a real-browser server-render-and-hydrate check, which the jsdom test at AppShell.test.tsx:1337 does not provide.
| {...rest} | ||
| ref={useMergedRefs(ref, shellRef)} | ||
| data-testid={dataTestId} | ||
| data-scroll-owner={scrollsDocument ? 'document' : 'content'} |
There was a problem hiding this comment.
Written after {...rest}, this replaces a caller's own data-scroll-owner: a caller's custom reaches the root on the base and reads content here. component:AppShell FR1 and architecture:public-component-api INV5 pass data inputs through today, so whether AppShell owns this attribute is part of the owner decision.
…names Byte-identical copies of each clip and still, named by pull request only.
…wing main region (#8111) Siblings: #8066, #6314. ## Why With `height="fill"`, a keyboard user can't scroll AppShell's main region when it holds nothing focusable: Tab skips it, so Page Down and the arrow keys do nothing. axe reports it as `scrollable-region-focusable`, a serious violation. ## What led to it Part of the touch-presentations program's work on AppShell scrolling (`spec:AST-094`), alongside #8066, which changes which element scrolls. Main was only the skip link's `tabIndex={-1}` target, which Tab never stops on. ## Options and tradeoffs - **Always make main a tab stop:** simple, but adds a stop even when main fits, and an extra stop before its first link. - **A stop only while main overflows, with Tab entry moving on to its first link or button:** what `spec:AST-025` FR12 asks; it costs measuring main, which happens only with `fill`. ## Why this choice The second follows FR12 and AppShell FR4: a viewport joins the tab order only while it overflows, and Shift+Tab skips it. A fitting main, and `height="auto"`, keep `tabIndex={-1}`. Only Tab focus draws main's ring, inset and following the surface's rounded corner; a click, the skip link or a script's focus draws none. Main has no inner content box, so an observer internal to AppShell watches main's direct children; the public `useScrollableArea` is unchanged, and its one-element form is a separate draft that waits on AST-025 OQ1. The AppShell record states the observation (PR1) and the ring (FR4), and drops the gap this closes. Main's stop is named by its `main` role alone, which the FR12 ruling allows for the single main. ## How to check it In Chromium, on the AppShell Playground story with `height="fill"` and 40 rows: 1. Press Tab until focus reaches main: a ring appears inside its edge, and Page Down scrolls it. Before, Tab left the page. 2. Add a link inside main: Tab lands on the link instead, and Shift+Tab from it skips main. 3. Click into main, or use the skip link: no ring. 4. Show fewer rows so main fits: main leaves the tab order. ## Evidence Before (main) and after (this PR):   Tab, then Shift+Tab, with a link inside; the ring:   - The browser test `AppShellMainKeyboard.a11y.browser.spec.ts` covers each step: 16 of 16 in Chromium, and 15 of 15 in WebKit, where the window-switch case is skipped. - Tab onto main exposes the landmark: Chromium's accessibility tree shows the focused node as `main`, not ignored and unnamed, so its role is announced; WebKit computes the same role. The test fails on main (Tab never reaches main) and when main is given a name. - AppShell's keyboard tests fail without the observer, or when a `fill`/`auto` switch doesn't restart it; each of the observer's 5 tests fails when its behavior is removed. - Local gates at 7d439ae: core UI suite (655 files), build, type check, lint, Storybook, the keyboard spec in both browsers, knowledge records and changeset coverage pass; the node project fails only on `cache.test.mjs` and `story-tree.test.ts`, which #8520 and #8509 fix. ## Writing rubric results - R1 narration: the PR1, FR4 and verification-map edits are present tense; the closed current-state gap is removed, and the glance row counts four gaps. - R2 research: none in the record. - R3 evidence: the new verification cells name test files, not changes. - R4 rejected alternatives: none added. - R5 change references: none. - R6 open questions: none touched; AST-025 OQ1 stays open with its options.
Review only this PR's change: vjeux/astryx@71601aa...64ba8ef
Why
On phones, browsers tie behaviors to the page's own scroll: the address bar shrinks as the page scrolls, and on iOS a tap on the status bar scrolls to the top. AppShell's default
height="fill"scrolls the main region inside a screen-high shell, so neither happens.What led to it
This is part of the touch-presentations program (
spec:AST-094), which makes native-feeling touch behavior the default on every touch device.height="auto"scrolls the page and gets both behaviors, but costs desktopsfill's independent regions, such as a side region that scrolls on its own, so apps had to pick one for every device.Options and tradeoffs
fillunder a finger: breaks apps that rely on main scrolling (spec:AST-017FR1, FR3).height="adaptive":fillwith a mouse or trackpad,autounder a finger, from the shared touch-presentation check.Why this choice
adaptivefollows the input rather than the width and changes no existing value:height?: 'fill' | 'auto' | 'adaptive'.data-scroll-owneron the root (contentordocument) tells code that restores scroll which element scrolls. Both are proposed additions tocomponent:AppShell, pending owner review, as is makingadaptivethe default in a later minor (height="fill"would restore today's behavior).How to check it
In the AppShell playground with 40 rows:
adaptive: scroll down. The header stays pinned, the page scrolls and the address bar shrinks; tap the status bar and the page returns to the top.fill: main scrolls inside the shell and the bars stay.adaptivekeeps independent regions, likefill.Evidence
fill(main) andadaptive(this PR), iOS 26.2 simulator, then Android 15 emulator:With the main thread blocked for 3 s, the page still scrolls:
The storyboard: the page scrolls under the pinned header, main never does: