Repository navigation
feat(Popover): presentation — a BottomSheet on a phone when asked, as iOS presents a popover - #8229
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
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/DateRangeInput | +4.5KB |
+32B |
| core/Popover | +2.5KB |
+2.5KB |
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 | 33.8KB | 33.9KB | +95B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
48 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 | 556,900 |
| RichTextEditor | Markdown parity: side by side | probe | light | 538,816 |
| 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 28 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.
Approval waits on an engineering owner's decision: presentation is a new Popover concept that component:Popover does not yet cover, so an engineering owner needs to approve DEC-6 (with its Presentation row and OQ1) or say what it must amend, recorded by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. That decision should also settle how the sheet relates to FR1 (one show, hide and toggle lifecycle), FR2 (content-first focus) and DEC-2 (the popover theme target), which DEC-6 leaves unamended. Independently of that decision, please fix:
- The content is mounted twice whenever the sheet can be the surface (in the hidden anchored layer and in the sheet), so callers get two instances with separate state, duplicated ids and two
data-testidelements. - With
hasLightDismiss={false}andhasEscapeDismiss={false}the sheet becomes arequiredalertdialog without the fallback close control, so read-only content cannot be closed. - A controlled
adaptivepopover swaps from the sheet to the anchored popover if the viewport crosses 768 px while it is open, and firesonOpenChange(true)from an effect. xstyle,classNameandstyle(and five other props) never reach the sheet, while the docs list onlyplacement,alignmentandwidthas not applying.
[Automated review]
| hasLightDismiss ? 'info' : hasEscapeDismiss ? 'form' : 'required' | ||
| } | ||
| data-testid={testId}> | ||
| {content} |
There was a problem hiding this comment.
The anchored layer (popover.render(...), line 799) also mounts content and keeps it mounted while closed, so whenever this sheet renders the caller's content exists twice: two instances with separate state and effects, duplicated explicit ids, and data-testid on two elements (with the sheet open on a Pixel 7 profile the page holds 8 checkboxes and 2 Apply buttons). Mounting one surface at a time, as DropdownMenu does, avoids it.
There was a problem hiding this comment.
Fixed in 4713e34: the content now exists once. The sheet holds it while the sheet is the surface (open, or the one an open would choose, through its exit), and the anchored layer holds it otherwise; the test id follows it. The new case "holds one instance of the content" failed before, with two Apply buttons.
| height="tall" | ||
| padding={padding} | ||
| purpose={ | ||
| hasLightDismiss ? 'info' : hasEscapeDismiss ? 'form' : 'required' |
There was a problem hiding this comment.
With both dismissals off this becomes required (no swipe, scrim click or Escape), and hasCloseButton is not passed through, so the fallback "Close popover" control the anchored surface keeps is gone: read-only content leaves phone users in an alertdialog they cannot close. Rendering the fallback close control in the sheet, or keeping Escape here, gives them a way out.
There was a problem hiding this comment.
Fixed in 4713e34: a sheet with neither light nor Escape dismissal keeps the anchored surface's hidden fallback close control (hasCloseButton, closeButtonLabel). The new case failed before.
| return; | ||
| } | ||
| if (isOpen && !popover.isOpen) { | ||
| if (isOpen && !popover.isOpen && resolvedPresentation !== 'bottom-sheet') { |
There was a problem hiding this comment.
For a controlled popover isSheetOpen follows the live query (line 451), so when a phone crosses 768 px while the sheet is open (rotating a Pixel 7) the sheet unmounts and this Effect opens the anchored popover, firing onOpenChange(true) with no user action (architecture:react-component-runtime INV2, INV3). Latching the surface chosen at open, as the uncontrolled path does, keeps the sheet until it closes.
There was a problem hiding this comment.
Fixed in 4713e34: a controlled popover latches the surface chosen as it opens and keeps it until it closes. A phone turned past 768 px keeps its sheet, and no onOpenChange(true) fires. The new case failed before.
| * The surface that holds the content. `'popover'` anchors it to the | ||
| * trigger. `'bottom-sheet'` presents it in a modal BottomSheet, labeled | ||
| * by `label`. `'adaptive'` presents the BottomSheet on a compact touch | ||
| * screen (a phone, in either orientation) and the anchored popover |
There was a problem hiding this comment.
The query is (max-width: 768px) and (pointer: coarse), so a phone wider than 768 px in landscape (a Pixel 7 at 863 px) gets the anchored popover; "in either orientation" overstates it. The list below also omits xstyle, className, style, role, isModal, hasCloseButton, closeButtonLabel and hasAutoFocus, which the sheet drops.
There was a problem hiding this comment.
Fixed in 4713e34: the docs now state the rule exactly (a coarse pointer and a viewport at most 768 px wide: a phone in portrait, and in landscape only while it stays that narrow) and list every prop a sheet does not use.
…presentation (#8273) ## Exact decision needed Whether callers can keep a stable component's existing surface when the component gains touch behavior, and through what API. ## Existing authority searched - `architecture:public-component-api` INV4 (keep derivable decisions internal), INV9 (released behavior changes under `spec:AST-017`), INV3 (one prop, one concept). - `spec:AST-002` FR2–FR3: derivable differences, thresholds, and tuning stay internal. - `spec:AST-017` FR3 and DEC-17: a functional default change names the prop that restores the old default; FR21 and FR27: an omitted experimental opt-in leaves stable behavior unchanged, which covers experimental surfaces only. - No current record requires a caller opt-out for touch behavior on a stable component. - Search terms: presentation, opt-out, touch default, restoring prop, adaptive, touch mode. Overlapping open pull requests: #8229 (Popover `presentation`), #8195 (DropdownMenu touch tracking), #8052 (AST-094 resolver), #8062 (shared touch resolver), #7977 and #8001 (touch sheets behind `experimental.presentation`). ## Decision - `architecture:public-component-api` INV16: a stable component that, under a coarse pointer, presents a different surface or switches how an open surface follows input takes one `presentation` prop, and a value such as `presentation="popover"` keeps its fine-pointer surface on every device. Touch behavior is the default where it serves touch users; the caller's explicit choice always wins. Every such touch mode is reachable by callers; thresholds and tuning stay internal. Touch styling and touch-only behavior other records define (press paint, the iOS text-size floor, a row swipe, a tooltip's touch trigger) stay with those records. - `spec:AST-017` FR3 and DEC-23: for a touch default, that `presentation` value is the restoring input the Changeset names. ## Rejected alternatives - Touch behavior that callers cannot turn off or pin. ## Claim scope and non-goals Caller control over touch presentations and modes on stable components. Which components adopt touch defaults, the touch behavior itself, and whether a given touch default is breaking stay with their owners. ## Evidence and implementation relationship Selector, MultiSelector, DropdownMenu, MoreMenu, ContextMenu, and the date and time inputs already take `presentation`. Related open work, not edited here: #8229 adds `presentation` to Popover (its `'popover'` default conforms); #8195 adds a touch-tracking mode to DropdownMenu, which INV16 requires callers to be able to reach; the `spec:AST-094` revision in #8052 carries the touch presentation resolver. No implementation is included. ## Writing rubric results - R1 narration: none. - R2 research in the record: none. - R3 evidence naming a change: none; verification names test states. - R4 rejected alternatives over one line or outside a decision: one, inside DEC-23, one line. - R5 pull request or issue references remaining: none. - R6 ruled open questions still listing options: none. ## Scope - [x] This PR contains specification records only. - [x] It records observable behavior and preserves equivalent internal implementations. - [x] Any named internal mechanism is intentionally public, names its dependent caller/system, and explains why an equivalent implementation would not satisfy the contract. - [x] Verification states the evidence layer and failure signal without prescribing CI job/workflow topology. - [x] It records one intentional durable decision, not a rescue for a separable tagalong. - [x] The canonical knowledge owner, affected current claims, and overlapping open work are named; any ownership collision is resolved or called out for human decision. - [x] Adjacent API, visual, accessibility, composition, and implementation facts remain outside scope unless this exact decision depends on them. - [x] Public text and artifacts contain no internal Meta context. ## Validation - `pnpm check:knowledge` (CI lint) - Prettier - `git diff --check`
… iOS presents a popover Popover takes `presentation` with the values DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector use. 'popover', the default, keeps the released anchored surface. 'bottom-sheet' presents the content in a modal BottomSheet labeled by `label`, at the sheet's tall height, inset by `padding`. 'adaptive' presents that sheet on a compact touch screen and the anchored popover elsewhere, which is how iOS presents a popover on an iPhone unless the app keeps it anchored. In a sheet, placement, alignment and width do not apply; light dismiss maps to the sheet's purpose (info, form without light dismiss, required with neither). The surface is chosen when the popover opens and stays until it closes; the trigger's expanded state and controlled element follow it. The sheet loads on demand (lazyPopoverBottomSheet), as DropdownMenu's does. component:Popover gains the Presentation concept and DEC-6, pending owner review, and OQ1 asks whether 'adaptive' becomes the default. The content exists once: the sheet holds it while it is the surface (open, or the one an open would choose, through its exit) and the anchored layer otherwise, so callers no longer get two instances, duplicated ids and two test ids. A sheet with neither light nor Escape dismissal keeps the anchored surface's hidden fallback close control. A controlled popover keeps the surface chosen as it opened until it closes, so a phone turned past the compact width keeps its sheet and onOpenChange(true) no longer fires from an effect. The docs list every prop a sheet does not use and state the compact rule exactly.
2582a33 to
4713e34
Compare
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: component:Popover DEC-6, with its Presentation row, verification row and OQ1, is a new proposal pending owner review, recorded by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and that decision should settle how the sheet relates to FR1 (one show, hide and toggle lifecycle), FR2 (content-first focus), DEC-2 (the popover theme target) and the hook-owned fallback close control. The earlier findings on duplicated content, the required sheet with no way out and the controlled sheet swap are fixed, each with a test that fails without the fix. Independently of the owner decision, please fix:
- Forward the caller's
xstyle,classNameandstyleto the sheet, which BottomSheet passes to its panel, and list the same props that do not apply in DEC-6, its row and the changeset as in the docs. - Describe
adaptivewithout the 768 px numbers, sincespec:AST-012FR9 onmaindefines compact as width belowmdor height below 500. - Set the controlled surface latch during render rather than in the layout Effect, as
architecture:react-component-runtimeINV1 and INV3 require. - Subscribe to the compact-touch media query only for
presentation="adaptive", so default popovers hold no viewport listener.
[Automated review]
| // surface's fallback close control, so it can always be closed. | ||
| const sheet = | ||
| isSheetOpen || resolvedPresentation === 'bottom-sheet' ? ( | ||
| <PopoverBottomSheet |
There was a problem hiding this comment.
The caller's xstyle, className and style never reach the sheet here, so one <Popover className="…"> is styled on a desktop and silently unstyled on a phone. architecture:public-component-api INV5 asks for supported style and class inputs to reach the element that owns the contract, and BottomSheet already forwards all three to its panel; passing the caller's three (not Popover's own viewport-fit styles) would keep them.
| * The surface that holds the content. `'popover'` anchors it to the | ||
| * trigger. `'bottom-sheet'` presents it in a modal BottomSheet, labeled | ||
| * by `label`. `'adaptive'` presents the BottomSheet on a compact touch | ||
| * screen (a coarse pointer and a viewport at most 768 px wide: a phone in |
There was a problem hiding this comment.
spec:AST-012 FR9 on main now defines compact as a viewport width below the md point or a height below 500, and says no resolver keeps its own numbers; a phone in landscape under 500 tall will count as compact once the shared resolver adopts it. Describing adaptive as "a compact touch screen" without the 768 px numbers (here and in the three doc rows) keeps the docs right through that change.
| const surface = | ||
| controlledSurface ?? (popover.isOpen ? 'popover' : resolvedPresentation); | ||
| if (surface !== controlledSurface) { | ||
| // eslint-disable-next-line @eslint-react/set-state-in-effect -- the surface is chosen once, as the controlled popover opens |
There was a problem hiding this comment.
The latch is derived from props, so the render where isOpen turns true commits isSheetOpen === false and this Effect repairs it before paint (architecture:react-component-runtime INV1), and the disable reasons name no external system (INV3). Setting controlledSurface during render when isOpen changes, and keeping this Effect for popover.show()/hide() only, would avoid both.
| const isControlled = isOpen !== undefined; | ||
| // The bottom-sheet presentation: the surface it resolves to now, and | ||
| // whether the sheet is the surface that is open. | ||
| const resolvedPresentation = useAdaptivePresentation(presentation); |
There was a problem hiding this comment.
This runs for every Popover, so each default and each closed instance now holds a matchMedia listener and re-renders when a phone crosses 768 px, though only adaptive reads it. Skipping the subscription unless presentation === 'adaptive' keeps closed and default popovers free of viewport observation, as PR1 intends.
|
/approve-spec 4713e34 |
github-actions
Bot
removed
the
needs:spec-owner-review
…pact definition, and only adaptive popovers listen Review fixes on the presentation change: - The caller's xstyle, className and style reach the sheet, which passes them to its panel, as they reach the anchored surface (they were dropped). The docs and changeset list what does not apply in a sheet: placement, alignment, width, role, isModal and hasAutoFocus. - `adaptive` reads compact from the compact definition (AST-012 FR9: narrower than the theme's md point or shorter than 500, with a coarse pointer), from the nearest Theme provider's width points, instead of the 768px query; the docs describe it without the old numbers. - Only an adaptive popover reads the theme and subscribes to that query; a default or bottom-sheet popover holds no viewport or document listener. - The controlled surface latch is set during render as `isOpen` turns on or off; the layout Effect only shows or hides the anchored popover. Tests: the compact query is read only for adaptive; a short landscape phone (height below 500) gets the sheet; caller className and style land on the sheet's panel. All fail on the previous head. Merges main for the compact definition. The Popover record is unchanged.
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec fe78c6b |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
An engineering owner has approved the presentation spec (DEC-6) for this head and the default popover path is unchanged, but one accessibility fix is needed before this can be approved. When an adaptive sheet is open and the screen stops being compact (for example an iPad mini turned to landscape), closing it unmounts the sheet before its exit finishes, so focus lands on the page body instead of returning to the trigger; please keep the sheet mounted through its exit once it has been the open surface, and add a test that closes it after the turn.
- Without a Theme provider,
adaptivemeasures compact against the defaultmdpoint rather than the registered root theme's, which AppShell follows for the same width points.
[Automated review]
| // it is the open one. A sheet with no dismissal keeps the anchored | ||
| // surface's fallback close control, so it can always be closed. | ||
| const sheet = | ||
| isSheetOpen || resolvedPresentation === 'bottom-sheet' ? ( |
There was a problem hiding this comment.
When an adaptive sheet is open and the screen stops being compact (an iPad mini turned from 744 × 1133 to 1133 × 744), closing it makes this condition false, so the sheet unmounts before BottomSheet's exit runs: in Chromium focus then lands on body instead of returning to the trigger (component:Popover AR4), and the exit motion is cut. Keeping the sheet, and the content in it (line 830), mounted until its exit finishes once it has been the open surface would fix it.
There was a problem hiding this comment.
Fixed in 0a53fa1. A sheet that has been the open surface now stays mounted after it closes, through its exit: its dialog closes while still in the document, BottomSheet hands focus back to the trigger, and the exit motion plays. The anchored popover opening releases it. The new test opens the adaptive sheet on a compact screen, turns the screen wide, closes it with Escape and ends the exit; the dialog closes while connected, focus is on the trigger, and the next open is the anchored popover with the content. It fails on fe78c6b.
| const isAdaptive = presentation === 'adaptive'; | ||
| // `use` may be called conditionally, unlike the other hooks. | ||
| const points = isAdaptive | ||
| ? use(ThemeContext)?.theme.__adaptations?.widthBreakpoints |
There was a problem hiding this comment.
Without a Theme provider this falls back to the default md, but architecture:theme-application has content with no provider follow the registered root theme, the path useThemeDefinition() gives AppShell for the same width points. In a separate React root under a root theme that overrides md, an adaptive Popover would measure compact (spec:AST-012 FR9) against a different point than AppShell.
There was a problem hiding this comment.
Fixed in 0a53fa1. Without a Theme provider, adaptive now reads the registered root theme's width points through useThemeDefinition, the path AppShell uses. The hook takes an enabled flag, so a default or bottom-sheet popover still reads and subscribes to nothing. The test registers a root theme with md: 900 and checks that the compact query follows it.
|
/approve-spec 131c988 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested for one small fix: the presentation design is owner-approved for this head, popovers without it are unchanged, and BottomSheet's slimmer frame renders the same pixels, but PopoverPresentation, the type of the new prop, is not exported from @astryxdesign/core/Popover (architecture:public-component-api INV1). Please add it to the type export in packages/core/src/Popover/index.ts, as DropdownMenu and Selector export their presentation types. Separately, a default sheet's first row starts 12 px down under BottomSheet's 24 px grab-bar fade, which is worth a follow-up. The fixing commit needs a fresh /approve-spec <full head SHA> from an engineering owner; the author may record it as one.
[Automated review]
| * a BottomSheet, or adaptive (a BottomSheet on a compact touch screen, the | ||
| * popover elsewhere). | ||
| */ | ||
| export type PopoverPresentation = AdaptivePresentation; |
There was a problem hiding this comment.
PopoverPresentation types the new public prop, but packages/core/src/Popover/index.ts exports only PopoverProps and PopoverTriggerRenderProps, so callers cannot import it from @astryxdesign/core/Popover or the root, while DropdownMenu and Selector export their presentation types. Please add it to the index's type export (architecture:public-component-api INV1).
| onOpenChange={setSheetOpen} | ||
| label={label ?? triggerText} | ||
| height="tall" | ||
| padding={padding} |
There was a problem hiding this comment.
With the default padding (step 3, 12 px) the first row starts under BottomSheet's grab bar, which floats over the top 24 px with the surface colour opaque to 60%; in Chromium at 390 px the top edge of a first checkbox fades. Not blocking, since DEC-8 decides the inset, but worth a follow-up so a sheet's top inset clears the bar.
From CodeBunny's review at 131c988: the new `presentation` prop's type is exported from @astryxdesign/core/Popover beside PopoverProps (public-component-api INV1).
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec e57d3b6 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested for one architecture fix: the presentation design is owner-approved for this head, popovers without it are unchanged, and BottomSheet's slimmer frame keeps its pixels, but Popover.tsx builds its own presentation hold (the controlled-surface latch and hasSheetBeenOpen), which architecture:layer-runtime INV14 on main assigns to the layer runtime. Please either move that hold into the layer runtime and use it from Popover, or, if an engineering owner prefers to keep it in Popover for now, record it beside the components INV14's gap paragraph already names. Separately, a default sheet's first row starts under BottomSheet's 24 px grab-bar fade, which is worth a follow-up. The fixing commit needs a fresh /approve-spec <full head SHA> from an engineering owner; the author may record it as one.
[Automated review]
| // A controlled popover opens on the surface its presentation resolves to | ||
| // as it opens, and keeps that surface until it closes: a phone rotated past | ||
| // the compact width while the sheet is open keeps its sheet. | ||
| const [controlledSurface, setControlledSurface] = useState< |
There was a problem hiding this comment.
This latch, with hasSheetBeenOpen below (:586-595, :662-675), is a Popover-owned presentation hold: it keeps the surface chosen at opening until the close settles. architecture:layer-runtime INV14 on main assigns that hold to the layer runtime ("A component that needs one of these uses the runtime's rather than building its own"), and its gap paragraph does not name Popover. Please move the hold into the layer runtime and use it here, or have an engineering owner record Popover's hold among INV14's current gaps.
| onOpenChange={setSheetOpen} | ||
| label={label ?? triggerText} | ||
| height="tall" | ||
| padding={padding} |
There was a problem hiding this comment.
Non-blocking: with the default padding (step 3, 12 px) the first row starts inside BottomSheet's grab bar, which floats over the top 24 px of the body, opaque to 60% and then fading, so the top of the first line reads washed out. DEC-8 decides the inset, so this is a follow-up: a sheet top inset that clears the bar.
…14 gap From CodeBunny's review at e57d3b6: Popover keeps the surface it chose from an opening until the close settles (its presentation hold) inside Popover, which INV14 assigns to the layer runtime. No shared hold exists yet, so the record's gap paragraph names Popover beside the components that already build their own layer mechanisms.
github-actions
Bot
added
the
needs:spec-owner-review
No conflicts. Brings architecture:layer-runtime INV14 and its gap paragraph.
…14 gap From CodeBunny's review at e57d3b6: Popover keeps the surface it chose from an opening until the close settles (its presentation hold) inside Popover, which INV14 assigns to the layer runtime. No shared hold exists yet, so the record's gap paragraph names Popover beside the components that already build their own layer mechanisms.
|
/approve-spec 268fa9b |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Recommending approval: presentation follows component:Popover DEC-6, the sheet's details (DEC-8), BottomSheet's slimmer frame and the layer-runtime line recording Popover's own presentation hold are approved by an engineering owner for this revision, and in Chromium default popovers and BottomSheet's pixels are unchanged. Two optional follow-ups: give the sheet a top inset that clears the grab bar (the first row starts 14 px below the top, inside the bar's 24 px fade), and align DEC-8 and AR2 with the docs on role, isModal, hasAutoFocus and the trigger-text name. Any further commit needs a fresh /approve-spec <full head SHA> from an engineering owner, which the author may record as one.
[Automated review]
| onOpenChange={setSheetOpen} | ||
| label={label ?? triggerText} | ||
| height="tall" | ||
| padding={padding} |
There was a problem hiding this comment.
Non-blocking: with the default padding (step 3, 12 px) the sheet's first row starts 14 px below the panel's top edge, inside BottomSheet's 24 px grab bar, which is opaque surface colour to 60 % and then fades (BottomSheetPanel.tsx:198-215). In Chromium at 390 px the heading's top reads washed out. A sheet top inset that clears the bar would fix it; DEC-8 decides the inset, so it is the owner's call.
| text when it has none, so the dialog always has a name. The sheet always | ||
| light-dismisses: a tap outside it or a swipe down closes it, whatever | ||
| `hasLightDismiss` says, so a finger always has a way out. Its fallback close | ||
| stays hidden, as in an anchored popover. Everything else follows Popover's |
There was a problem hiding this comment.
Non-blocking: "Everything else follows Popover's existing props" reads as if role, isModal and hasAutoFocus still apply in a sheet, while the prop docs, JSDoc and changeset say they do not, and the modal sheet always takes focus. Naming them beside placement, alignment and width would align DEC-8 with the shipped behaviour; AR2's "does not synthesize a label" could likewise be scoped to the anchored surface.
| expect(trigger()).toHaveAttribute('aria-haspopup', 'dialog'); | ||
| }); | ||
|
|
||
| it('a sheet with neither dismissal renders no more elements than the anchored popover', async () => { |
There was a problem hiding this comment.
Non-blocking: the name says the sheet renders no more elements than the anchored popover, but the assertion at line 895 expects exactly one more (the edge tint, within the open-layer allowance). Renaming the test to match would keep it from misleading a later reader.
…keyboard notification export
uikit/uikit.test.ts: main's leavesOf (a one-line export is a row, sourced
or not) keeps this branch's check of values bound from an exported group
(const {curve: move} = group). uikit/timing.ts merged cleanly.
|
Popover loads its bottom sheet with The cost of loading it with Popover, measured with the size report's builder ( This branch makes it static:
Whether to take it is your call. |
User need
On a phone, a Popover is a small box beside its trigger; iOS presents a popover as a sheet on an iPhone. A product wanting that composes BottomSheet itself (the Adaptive Popover Recipe story), wiring trigger, open state and label twice; the menus already take
presentation.Evidence
iOS 26.2 Safari, Adaptive Presentation story, a tap on Filter:
aria-expandedtrue,aria-controlsin the sheet.A drag down (the grey dot is the finger) closes it and collapses the trigger:
Problem and solution fit
Popover takes
presentationwith the same values:'popover'(default, unchanged),'bottom-sheet', and'adaptive'(the sheet on a compact touch screen, the popover elsewhere). One trigger, open state,onOpenChangeand content instance; the trigger'saria-expandedandaria-controlsfollow the open surface, chosen when it opens and kept until it closes.Current specification
component:PopoverDEC-6 and DEC-8 (cixzhang, 2026-10-25) decidepresentationand the sheet it presents; this implements both and adds a Touch sheet anatomy row delegating to BottomSheet'sbottom-sheettarget (architecture:component-theming-surfaceINV3). Sibling #8182 edits the same files.Public delta and caller burden
presentation?: 'popover' | 'bottom-sheet' | 'adaptive', default'popover': existing callers see no change.label, else by the trigger's text; tall, inset bypadding, styled byxstyle,classNameandstyle. It always light-dismisses (a tap outside, a swipe down), Escape followshasEscapeDismiss, and the fallback close stays hidden until reached.role,isModalandhasAutoFocusdo not apply.Design and alternatives
spec:AST-017FR60 (DEC-29) andarchitecture:component-style-authoringINV7: BottomSheet's frame drops three elements and gains no part or target, so the open sheet on a phone renders as many elements as the anchored popover with a mouse (178 against 178, with the default dismissals and with both off).Scope
Writing rubric results
Testing
At e561e4f:
pnpm vitest run packages/core/src/Popover packages/core/src/BottomSheet packages/core/src/theme packages/core/src/Button packages/core/src/DropdownMenu packages/core/src/Selector packages/core/src/ContextMenu packages/core/src/Tokenizer: 2726 pass, includingPopoverPresentation.test.tsx(24: a closed sheet observing nothing; content state across openings; a role="none" trigger advertisingdialog; a sheet light-dismissing by a tap outside and a swipe down whateverhasLightDismisssays; Escape followinghasEscapeDismiss; the hidden fallback close kept in the sheet; the trigger's text naming a sheet without a label; a sheet with both dismissals off rendering no more elements than the anchored popover) andBottomSheetEdgeTint.test.tsx(the tint on the panel, with no element of its own, and the declarations WebKit samples).document.body.querySelectorAll('*').length(rest / open / after close), desktop 1280 px mouse vs phone 390 px coarse pointer: Adaptive Presentation 178 / 178 / 178 vs 178 / 178 / 178; with both dismissals off 178 vs 178. In jsdom: 16 vs 16 with the default dismissals, without light dismiss, and with neither. Every other Popover story is unchanged.::after, as it did with the element. A control with the tint hidden reads the same in this simulator, so the unit test pins the sampled declarations.pnpm exec vitest run --project node packages/cli/test: 309 pass.pnpm build, thenpnpm typecheck(every package), eslint, prettier,pnpm check:knowledge,pnpm check:changesetsandnode scripts/check-sync.jspass.playwright.config.ts): Chromium 61, WebKit 34 (in the Playwright container), all pass.