Repository navigation
refactor(Layer): one presentation hold keeps a layer's chosen presentation until its close settles - #8587
refactor(Layer): one presentation hold keeps a layer's chosen presentation until its close settles#8587cixzhang wants to merge 2 commits into
Conversation
…ation until its close settles architecture:layer-runtime INV14 gives the layer runtime the presentation hold: keeping a layer's chosen presentation from the open until the close settles. Layer/presentationHold.ts (internal) is that one job: createPresentationHold for code that chooses at an open event, and usePresentationHold for a choice taken during render; the component releases it when its close settles. Popover adopts it for its three latches, with no behavior change: the controlled surface (taken as isOpen turns on, let go as it turns off), the sheet kept mounted until its dialog has closed, and the touch-or-pointer presentation its close plays in (usePopoverSurfaceMotion). Not the input hold (inputHold.ts). Not exported; a test locks that. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
needs:spec-owner-review
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsPopover (@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 | +2.6KB |
+1B |
| core/ChatLayout | +15.7KB |
+39B |
| core/Popover | +12.7KB |
+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.3KB | 34.3KB | +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 can't be approved yet: it needs an engineering owner's decision on a spec addition, and three changes. Through the stacked exit-copy work, the diff against main adds INV13 ("One exit copy per closing layer") to the current architecture:layer-runtime record. An engineering owner records approval or rejection by commenting /approve-spec followed by the full head commit SHA on this pull request, and the author may do so if they are an engineering owner.
- Land the parent pull requests (#8187, #8342 and the stack beneath them) and rebase, or retarget the base, so the diff carries only the 295-line presentation-hold commit instead of 38,390 authored lines.
- Add
touchHoldto theuseCallbackdependency list atpackages/core/src/Popover/usePopoverSurfaceMotion.ts:134, which currently fails the Lint check. - Update the Current gaps paragraph and Owning code in
docs/architecture/layer-runtime.md, which still say no shared presentation hold exists and that Popover builds its own.
[Automated review]
| source: boxOf(source), | ||
| })); | ||
| }, | ||
| [motionFor], |
There was a problem hiding this comment.
This callback calls touchHold.take(isTouch) (line 118) but lists only motionFor. Under CI's strict lint, @eslint-react/exhaustive-deps is an error, and the Lint job fails here. touchHold is stable, so listing it changes nothing at runtime:
[motionFor, touchHold],| INV14 and INV15 are not met yet, and `spec:AST-003` does not cover them. The | ||
| runtime owns the shared exit copy (INV13, `placeLayerExitCopy`); no shared | ||
| presentation hold, runtime backdrop tint or lift module exists, Dialog, Drawer, | ||
| BottomSheet, Lightbox, MobileNav, ToastViewport, and Selector each build their | ||
| own top-layer mounting, exit, or backdrop, Popover builds its own | ||
| presentation hold for its sheet presentation, and Popover latches its chosen | ||
| anchored presentation until the close settles. |
There was a problem hiding this comment.
Once this pull request's presentation-hold commit lands, "no shared presentation hold … exists" and "Popover builds its own presentation hold for its sheet presentation" are no longer true, because Popover's latches go through Layer/presentationHold.ts. Please say that the runtime owns the presentation hold and Popover uses it, keep the seven other components as the remaining gap, and add Layer/presentationHold.ts to Owning code.
…the record names the shared hold usePopoverSurfaceMotion's returned callback reads touchHold, so it is in its dependency list (lint). architecture:layer-runtime's current-gaps paragraph no longer says no shared presentation hold exists or that Popover builds its own: the runtime owns it (Layer/presentationHold.ts) and Popover uses it; Owning code and verified_by name it.
|
/approve-spec 99b0ba8 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
One small change is needed before approval: this pull request edits docs/architecture/layer-runtime.md, so please add a ## Writing rubric results section to the description with one line for each of R1–R6 from docs/contributing/spec-writing.md. The code needs no changes: Popover's three latches move onto the internal presentation hold with no behavior change, the Layer and Popover tests pass, Chromium renders the sheet and touch states exactly as before, and the record change is approved at this head. The failed Review signal job refused a stale 208-file count, so re-running it should publish review-required for this head.
[Automated review]
There was a problem hiding this comment.
This looks ready to approve: Popover's three presentation latches move onto the layer runtime's internal presentation hold with no behavior change, the unchanged Layer and Popover tests pass, Chromium renders the sheet and touch states exactly as before, and the record change is approved at this head with the writing rubric results now in the description. One housekeeping step remains: the Review signal check failed on a stale 208-file count when the base changed, so re-running it should publish review-required for this head.
[Automated review]
github-actions
Bot
removed
the
needs:design-review
Stacked on #8187 (Popover's touch presentation) and #8342 (the input hold,
Layer/inputHold): the base branchstack/8587-baseholds both, so the diff is this change alone; it retargets tomainas they land.What it does
One internal module with one job, the layer runtime's presentation hold (
architecture:layer-runtimeINV14): keep a layer's chosen presentation from the open until its close settles.packages/core/src/Layer/presentationHold.ts:usePresentationHold({isOpen, presentation}) → {presentation, isHeld, release}. The render in whichisOpenturns on holdspresentationas it is then. It stays held, whateverpresentationbecomes, until the component callsrelease()when its close has settled.createPresentationHold(): the same hold for code that chooses at an open event (take,current,isHeld,release).It decides nothing about timing; the component says when its close has settled. It isn't the input hold (
inputHold.ts, which drops input while a presentation animates). It isn't exported from the package or the Layer barrel (presentationHold.test.tsxlocks that). It contains no component names.Record
architecture:layer-runtime's current-gaps paragraph no longer says no shared presentation hold exists or that Popover builds its own; Owning code andverified_bynameLayer/presentationHold.ts. INV14 itself is unchanged.Writing rubric results
R1 narration: none; the current-gaps sentence and the Owning code entry state what the runtime owns now, in the present tense.
R2 research: none in the record.
R3 evidence: no evidence, basis or verification cell changed;
verified_bynames a test file.R4 rejected alternatives: none added.
R5 change references: none.
R6 open questions: none touched.
First adopter: Popover
All three of Popover's latches move onto it:
isOpenturns on, let go as it turns off, so a phone rotated past the compact width keeps its sheet while open.<dialog>closes.usePopoverSurfaceMotion): taken at the open, read and let go as the close begins.Other components adopt it one pull request each: MobileNav (#8590) and Selector/MultiSelector (#8591) are open; Dialog, Lightbox and ToastViewport follow their touch-presentation pull requests, and BottomSheet its sheet stack. Known exception: Drawer follows
sidewhile open; adopting the shared hold would freeze it at open, a public behavior change for a later decision, so Drawer keeps its own exit latch.No behavior change
Popover's existing tests are the oracle and pass unchanged:
Popover.test.tsx,PopoverPresentation.test.tsx(the sheet latch, rotation and reopen cases) and the Layer suite, 370 tests in all, plus the module's own tests. The real-browser check is Popover's touch-presentation specs, which this PR's CI runs because the change touches the Popover folder.Size (gzip KB, minified)
Method: a fresh
packages/coredist at the base and at this head; an entry re-exporting the component; esbuild bundle, minify, splitting, ESM, packages external. Desktop is the static-import closure; phone is all chunks.Popover gains 0.16 KB for the shared module; later adopters reuse it.
Testing
pnpm vitest run --project ui packages/core/src/Layer packages/core/src/Popover: 370 pass.[chore]changeset (internal, no behavior change).Co-authored with vjeux (the latches move from his Popover code).