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:design-review
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report
The exact-head preview is not available yet. Generated by PR Enrichment workflow | View current CI run |
…wing main region With `height="fill"`, main becomes a sequential tab stop while its content overflows (spec:AST-025 FR12), and Tab focus on it draws the standard ring, inset (component:AppShell FR4). AppShell measures main through an internal observer that watches main's direct children, since main has no inner content box; the public useScrollableArea is unchanged. The AppShell record states the observation in PR1 and the ring in FR4.
… content box When the same element takes both getViewportProps and getContentProps, the hook watches that element's direct children's sizes, following them as they change, so a scroller whose content has no wrapper of its own is measured without adding one. Its mutation signals use the shared observers, one per set of options for every scroll area on the page.
![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 for a spec decision: the one-element useScrollableArea shape is the open question spec:AST-025 OQ1, and the current AST-025 FR2/FR5 and the useScrollableArea module's DEC-1 still describe a separate content box inside the viewport, so an engineering owner needs to settle OQ1 (amending those claims) or decline the shape; the AppShell PR1 and FR4 changes carried from #8111 need the same approval. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whichever way it goes, two things remain:
- AppShell's
observeMainOverflowduplicates the observation the hook gains here, and AST-025 IR1 asks component integrations to use the shared hook instead of a parallel detector, so the branch should end with one. - The branch still contains #8111's commit; land that first and rebase so this diff is only the hook change.
[Automated review]
| const childObservers = new Map<Element, () => void>(); | ||
| const syncChildObservers = () => { | ||
| const children = new Set(Array.from(main.children)); | ||
| for (const [child, stop] of childObservers) { | ||
| if (!children.has(child)) { | ||
| stop(); | ||
| childObservers.delete(child); | ||
| } | ||
| } | ||
| for (const child of children) { | ||
| if (!childObservers.has(child)) { | ||
| childObservers.set(child, observeResize(child, scheduleMeasure)); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
This child-observer loop is the same one this PR adds to useScrollableArea (useScrollableArea.ts:390-407), with identical mutation options. AST-025 IR1 asks component integrations to consume the shared hook rather than run a parallel detector; once OQ1 is settled, AppShell's main can use the hook (its one-element form, or an internal content box) and this module can go.
| }); | ||
|
|
||
| it('does not set up ResizeObserver in fill mode', () => { | ||
| it('does not observe the header in fill mode', () => { |
There was a problem hiding this comment.
This was the guard for AppShell the current PR1 ("observes nothing in fill"); it now asserts the proposed PR1. It should change only together with an approved PR1.
204914e to
065e544
Compare
There was a problem hiding this comment.
Approval is on hold for a spec decision. The one-element useScrollableArea shape is the open question spec:AST-025 OQ1, and AST-025's content-box claims (FR2/FR5) and the useScrollableArea module's DEC-1 still describe a separate content box inside the viewport. An engineering owner needs to settle OQ1 (amending those claims) or decline the shape, and to approve or reject the AppShell PR1 and FR4 changes this branch carries from #8111, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whichever way that goes, three things remain:
- AppShell's
observeMainOverflowrepeats the observation this PR adds to the hook, and AST-025 IR1 asks component integrations to use the shared hook rather than a parallel detector. - The description needs
## Writing rubric resultsfor theAppShell.spec.mdedit (#8111 has them), unless that edit leaves this branch. - Land #8111 first and rebase, so this diff is the hook change alone.
[Automated review]
Depends on #8111, for its shared mutation observers and the keyboard delegation of a viewport that is its own content box. Review only this PR's change: vjeux/astryx@399ea1a...065e544
This waits on
spec:AST-025OQ1, which leaves the public one-element shape open: it doesn't ship until the spec owner settles it. Sibling: #6314 hardens the same hook.Why
useScrollableAreameasures a viewport around a separate content box. A scroller whose content sits directly inside it, like AppShell's main region, has no content box: its content grows without resizing it, so the hook never sees its overflow change. Such a scroller has to add a wrapper element just to be measured.What led to it
Split out of #8111 at the spec owner's request, one solution per pull request. #8111 measures AppShell's main through an observer internal to AppShell; this offers the same measurement through the public hook.
Options and tradeoffs
Why this choice
It measures what a wrapper would without changing the DOM, and leaves the two-element shape as it is: with a separate content box, only that box is watched. The hook's mutation signals move to the shared observers, one per set of options for every scroll area on the page.
How to check it
getViewportPropsandgetContentProps, and let a child grow until it overflows:state.block.isScrollableturns true.Evidence
useScrollableArea.singleElement.test.tsx: the first two cases fail on the hook without this change; the third passes either way.cache.test.mjsandstory-tree.test.ts, which fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520 and test(storybook): run the story-tree check where Storybook is built, and bring the tree back to its shape #8509 fix.