-
Notifications
You must be signed in to change notification settings - Fork 465
feat(headless): hold Popover contents while it closes #9365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| --- | ||
| --- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ export interface PopoverProps { | |
| onOpenChange?: (open: boolean) => void; | ||
| placement?: Placement; | ||
| sideOffset?: number; | ||
| alignOffset?: number; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Add coverage for This new public property has no supplied regression test. Add a test that verifies a non-zero As per coding guidelines, “Unit tests are required for all new functionality.” 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| modal?: boolean; | ||
| /** | ||
| * Where focus lands when the popup opens. | ||
|
|
@@ -47,7 +48,14 @@ export interface PopoverProps { | |
|
|
||
| function PopoverInner(props: PopoverProps) { | ||
| const nodeId = useFloatingNodeId(); | ||
| const { placement: placementProp = 'bottom', sideOffset = 4, modal = false, initialFocus = 'auto', children } = props; | ||
| const { | ||
| placement: placementProp = 'bottom', | ||
| sideOffset = 4, | ||
| alignOffset = 0, | ||
| modal = false, | ||
| initialFocus = 'auto', | ||
| children, | ||
| } = props; | ||
|
|
||
| const [open, setOpen] = useControllableState(props.open, props.defaultOpen ?? false, props.onOpenChange); | ||
|
|
||
|
|
@@ -71,7 +79,7 @@ function PopoverInner(props: PopoverProps) { | |
| onOpenChange: setOpen, | ||
| placement: placementProp, | ||
| middleware: [ | ||
| offset(sideOffset), | ||
| offset({ mainAxis: sideOffset, alignmentAxis: alignOffset }), | ||
| flip({ | ||
| crossAxis: placementProp.includes('-'), | ||
| fallbackAxisSideDirection: 'end', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import { cleanup, render, screen } from '@testing-library/react'; | ||
| import { afterEach, describe, expect, it } from 'vitest'; | ||
|
|
||
| import { Freeze } from './freeze'; | ||
|
|
||
| afterEach(() => { | ||
| cleanup(); | ||
| }); | ||
|
|
||
| describe('Freeze', () => { | ||
| it('renders children while not frozen', () => { | ||
| render(<Freeze frozen={false}>Acme</Freeze>); | ||
|
|
||
| expect(screen.getByText('Acme')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('holds the committed DOM when children change while frozen', () => { | ||
| const { rerender } = render(<Freeze frozen={false}>Acme</Freeze>); | ||
|
|
||
| rerender(<Freeze frozen>Globex</Freeze>); | ||
|
|
||
| expect(screen.getByText('Acme')).toBeInTheDocument(); | ||
| expect(screen.queryByText('Globex')).toBeNull(); | ||
| }); | ||
|
|
||
| it('keeps the held DOM visible', () => { | ||
| const { rerender } = render(<Freeze frozen={false}>Acme</Freeze>); | ||
|
|
||
| rerender(<Freeze frozen>Globex</Freeze>); | ||
|
|
||
| expect(screen.getByText('Acme')).toBeVisible(); | ||
| }); | ||
|
|
||
| it('keeps the held DOM visible across further updates while frozen', () => { | ||
| const { rerender } = render(<Freeze frozen={false}>Acme</Freeze>); | ||
|
|
||
| rerender(<Freeze frozen>Globex</Freeze>); | ||
| rerender(<Freeze frozen>Initech</Freeze>); | ||
|
|
||
| expect(screen.getByText('Acme')).toBeVisible(); | ||
| }); | ||
|
|
||
| it('commits the pending children once unfrozen', () => { | ||
| const { rerender } = render(<Freeze frozen={false}>Acme</Freeze>); | ||
|
|
||
| rerender(<Freeze frozen>Globex</Freeze>); | ||
| rerender(<Freeze frozen={false}>Globex</Freeze>); | ||
|
|
||
| expect(screen.getByText('Globex')).toBeInTheDocument(); | ||
| expect(screen.queryByText('Acme')).toBeNull(); | ||
| }); | ||
|
|
||
| it('holds state updates raised from inside the frozen subtree', () => { | ||
| function Counter({ count }: { count: number }) { | ||
| return <span>count: {count}</span>; | ||
| } | ||
|
|
||
| const { rerender } = render( | ||
| <Freeze frozen={false}> | ||
| <Counter count={0} /> | ||
| </Freeze>, | ||
| ); | ||
|
|
||
| rerender( | ||
| <Freeze frozen> | ||
| <Counter count={1} /> | ||
| </Freeze>, | ||
| ); | ||
|
|
||
| expect(screen.getByText('count: 0')).toBeInTheDocument(); | ||
|
Comment on lines
+53
to
+70
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Test an actual state update from the frozen subtree.
As per coding guidelines, “Unit tests are required for all new functionality” and tests must verify edge cases. 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| 'use client'; | ||
|
|
||
| import * as React from 'react'; | ||
|
|
||
| /** | ||
| * Never settles. Throwing it suspends the enclosing boundary indefinitely: React keeps | ||
| * rendering the subtree but holds the commit, so the DOM keeps painting its last frame. | ||
| */ | ||
| const never = new Promise<never>(() => {}); | ||
|
|
||
| function Suspend(): null { | ||
| // eslint-disable-next-line @typescript-eslint/only-throw-error -- Suspending is React's thrown-thenable protocol, not an error. `React.use()` would say this more plainly but needs React 19.2; this package supports React 18. | ||
| throw never; | ||
| } | ||
|
|
||
| export interface FreezeProps { | ||
| /** While `true`, the DOM below holds whatever it last committed. */ | ||
| frozen: boolean; | ||
| children?: React.ReactNode; | ||
| } | ||
|
|
||
| /** | ||
| * Holds its subtree's DOM at the last committed frame while `frozen`. Renders keep | ||
| * happening, they just don't reach the DOM; the pending one commits when `frozen` flips | ||
| * back to `false`. | ||
| * | ||
| * Use it to stop content from visibly changing under an exit animation — a popover that | ||
| * closes because the thing it was showing changed would otherwise swap its contents on the | ||
| * way out. | ||
| */ | ||
| export function Freeze({ frozen, children }: FreezeProps) { | ||
| const contentRef = React.useRef<HTMLDivElement | null>(null); | ||
|
|
||
| // Hold onto the node ourselves rather than reading a plain ref: hiding a boundary's children | ||
| // detaches their refs, so by the time the effect below runs a normal ref reads `null`. | ||
| const setContent = React.useCallback((node: HTMLDivElement | null) => { | ||
| if (node) { | ||
| contentRef.current = node; | ||
| } | ||
| }, []); | ||
|
|
||
| // React hides a suspended boundary's host children with `display: none !important`, which is | ||
| // the opposite of what this is for. Undo it on the commit that applies it: insertion effects | ||
| // run after the boundary's mutation and before paint, so the held frame never blinks out. | ||
| // `display: contents` is also what the wrapper renders with, so React puts it back on unfreeze | ||
| // and the wrapper stays out of the layout it is spliced into. | ||
| React.useInsertionEffect(() => { | ||
| if (frozen) { | ||
| contentRef.current?.style.setProperty('display', 'contents'); | ||
| } | ||
| }, [frozen]); | ||
|
|
||
| return ( | ||
| <React.Suspense fallback={null}> | ||
| {frozen ? <Suspend /> : null} | ||
| <div | ||
| ref={setContent} | ||
| style={{ display: 'contents' }} | ||
| > | ||
| {children} | ||
| </div> | ||
| </React.Suspense> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert each required intermediate open state.
These tests can pass if the menu never opens. The second Popover test can also pass if the first Escape closes the Popover.
packages/headless/src/primitives/menu/menu.test.tsx#L708-L713: Assert thatShareis open afterArrowRightand before Escape.packages/headless/src/primitives/menu/menu.test.tsx#L742-L746: Assert thatActionsis open after the click and before Escape.packages/headless/src/primitives/menu/menu.test.tsx#L753-L757: After the first Escape, assert thatActionsis closed andOpen popoverremains open before the second Escape.As per coding guidelines, unit tests must cover new functionality and edge cases.
📍 Affects 1 file
packages/headless/src/primitives/menu/menu.test.tsx#L708-L713(this comment)packages/headless/src/primitives/menu/menu.test.tsx#L742-L746packages/headless/src/primitives/menu/menu.test.tsx#L753-L757🤖 Prompt for AI Agents
Source: Coding guidelines