-
-
Notifications
You must be signed in to change notification settings - Fork 768
refactor(react): one vocabulary and simpler plumbing for portal elements #3052
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
Changes from all commits
e0e3e90
3773025
9b2b740
3c9c0b6
72fce8f
0fdf553
543523d
f69f5c9
85b9029
473642d
4ca626d
9e7dfa5
bbb5ddf
ceb93bd
3eb01f7
c63532a
ea7aef9
17eca72
355ddc4
7c50b00
4150a35
4dbe297
4d3a67c
9cfade6
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 |
|---|---|---|
|
|
@@ -8,9 +8,9 @@ import { assertEmpty, mergeCSSClasses } from "@blocknote/core"; | |
| import { ComponentProps } from "@blocknote/react"; | ||
| import { createContext, forwardRef, useContext } from "react"; | ||
|
|
||
| const PortalRootContext = createContext<HTMLElement | null | undefined>( | ||
| undefined, | ||
| ); | ||
| // Hands the `portalElement` prop from `Popover` (the root) down to | ||
| // `PopoverContent`, where Ariakit takes it. | ||
| const PopoverPortalElementContext = createContext<HTMLElement | null>(null); | ||
|
|
||
| export const PopoverTrigger = forwardRef< | ||
| HTMLButtonElement, | ||
|
|
@@ -31,7 +31,7 @@ export const PopoverContent = forwardRef< | |
|
|
||
| assertEmpty(rest); | ||
|
|
||
| const portalRoot = useContext(PortalRootContext); | ||
| const portalElement = useContext(PopoverPortalElementContext); | ||
|
|
||
| return ( | ||
| <AriakitPopover | ||
|
|
@@ -40,7 +40,10 @@ export const PopoverContent = forwardRef< | |
| className || "", | ||
| variant === "panel-popover" ? "bn-ak-panel-popover" : "", | ||
| )} | ||
| portalElement={portalRoot ?? undefined} | ||
| // Ariakit falls back to a body-appended div for a missing element, so | ||
| // don't portal at all until there is one (editor not mounted yet). | ||
| portal={portalElement !== null} | ||
| portalElement={portalElement} | ||
| ref={ref} | ||
| > | ||
| {children} | ||
|
|
@@ -51,7 +54,15 @@ export const PopoverContent = forwardRef< | |
| export const Popover = ( | ||
| props: ComponentProps["Generic"]["Popover"]["Root"], | ||
| ) => { | ||
| const { children, open, onOpenChange, position, portalRoot, ...rest } = props; | ||
| const { | ||
| children, | ||
| open, | ||
| onOpenChange, | ||
| position, | ||
| portalElement, | ||
| preventFocusOnOpen: _preventFocusOnOpen, // unused; see Menu.tsx | ||
|
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 | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🤖 get_repo_knowledge executed:
Length of output: 2259 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- changed files and nearby definitions ---'
for f in packages/ariakit/src/popover/Popover.tsx packages/ariakit/src/toolbar/ToolbarSelect.tsx; do
printf '\n--- %s ---\n' "$f"
cat -n "$f" | sed -n '1,180p'
done
printf '\n--- related prop declarations and component usages ---\n'
rg -n --glob '*.ts' --glob '*.tsx' \
'preventFocusOnOpen|autoFocusOnShow|AriakitPopover|AriakitSelectPopover' \
packages/ariakit packages | head -240Repository: TypeCellOS/BlockNote Length of output: 10752 🏁 Script executed: #!/bin/bash
set -eu
for f in packages/ariakit/src/popover/Popover.tsx packages/ariakit/src/toolbar/ToolbarSelect.tsx; do
printf '\n--- %s ---\n' "$f"
cat -n "$f" | sed -n '1,180p'
done
printf '\n--- related symbols ---\n'
rg -n --glob '*.ts' --glob '*.tsx' 'preventFocusOnOpen|autoFocusOnShow|AriakitPopover|AriakitSelectPopover' packages/ariakitRepository: TypeCellOS/BlockNote Length of output: 6633 🏁 Script executed: pwd
printf '%s\n' "$(git ls-files packages/ariakit/src/popover/Popover.tsx packages/ariakit/src/toolbar/ToolbarSelect.tsx)"Repository: TypeCellOS/BlockNote Length of output: 265 🤖 get_repo_knowledge executed:
Length of output: 1544 🏁 Script executed: #!/bin/bash
set -eu
cat -n packages/ariakit/src/popover/Popover.tsx | sed -n '1,140p'
cat -n packages/ariakit/src/toolbar/ToolbarSelect.tsx | sed -n '1,140p'Repository: TypeCellOS/BlockNote Length of output: 5293 🌐 Web query:
💡 Result: In Ariakit, the Popover component inherits from the Dialog component [1]. The behavior concerning focus when the popover opens is controlled via the autoFocusOnShow prop [2]. 1. autoFocusOnShow: This prop determines whether the popover takes focus when it opens [3][2]. It defaults to true [2]. If you wish to prevent the popover from automatically receiving focus, you can set autoFocusOnShow={false} on the Popover component [4]. 2. preventFocusOnOpen: There is no prop named preventFocusOnOpen in the Ariakit Popover API. The functionality to disable automatic focus is handled entirely by setting autoFocusOnShow to false [2][4]. If you disable autoFocusOnShow, the popover will not automatically focus its content upon opening. This is a common pattern when you want to avoid stealing focus from the trigger element or another part of the page [4]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- Ariakit dependency declarations ---'
rg -n --glob 'package.json' --glob 'pnpm-lock.yaml' --glob 'yarn.lock' --glob 'package-lock.json' \
'"`@ariakit/react`"|`@ariakit/react`@' .
printf '%s\n' '--- shared component contract and existing Ariakit menu mapping ---'
rg -n -A12 -B8 'preventFocusOnOpen' packages/react/src/editor/ComponentsContext.tsx packages/ariakit/src/menu/Menu.tsxRepository: TypeCellOS/BlockNote Length of output: 7157 🌐 Web query:
💡 Result: In Ariakit 0.4.24, the Citations:
🏁 Script executed: #!/bin/bash
set -eu
for url in \
'https://unpkg.com/@ariakit/react@0.4.24/ts/popover/popover.d.ts' \
'https://unpkg.com/@ariakit/react@0.4.24/ts/select/select-popover.d.ts' \
'https://unpkg.com/@ariakit/react@0.4.24/ts/dialog/dialog.d.ts'
do
printf '\n--- %s ---\n' "$url"
curl -fsSL "$url" | rg -n -C3 'autoFocusOnShow|SelectPopover|PopoverProps' || true
doneRepository: TypeCellOS/BlockNote Length of output: 528 Honor Both adapters discard this shared prop. In 📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| ...rest | ||
| } = props; | ||
|
|
||
| assertEmpty(rest); | ||
|
|
||
|
|
@@ -61,9 +72,9 @@ export const Popover = ( | |
| setOpen={onOpenChange} | ||
| placement={position} | ||
| > | ||
| <PortalRootContext.Provider value={portalRoot}> | ||
| <PopoverPortalElementContext.Provider value={portalElement}> | ||
| {children} | ||
| </PortalRootContext.Provider> | ||
| </PopoverPortalElementContext.Provider> | ||
| </AriakitPopoverProvider> | ||
| ); | ||
| }; | ||
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 | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: TypeCellOS/BlockNote
Length of output: 28623
🤖 get_repo_knowledge executed:
get_repo_knowledge TypeCellOS/BlockNote /tmp/coderabbit-repo-knowledge/typecellos-blocknote-375222acLength of output: 1588
🏁 Script executed:
Repository: TypeCellOS/BlockNote
Length of output: 46813
Preserve editor focus when this mobile popover opens.
Pass
preventFocusOnOpen={useUIMode() === "mobile"}toComponents.Generic.Popover.Root. The Mantine adapter otherwise allows focus to move into the popover, which can blur the editor and dismiss the on-screen keyboard. Add a touch regression test for this behavior.🤖 Prompt for AI Agents