refactor(react): one vocabulary and simpler plumbing for portal elements - #3053
Draft
YousefED wants to merge 2 commits into
Draft
refactor(react): one vocabulary and simpler plumbing for portal elements#3053YousefED wants to merge 2 commits into
YousefED wants to merge 2 commits into
Conversation
Reworks the portal consolidation from #3046, keeping its model — portal elements registered on the editor, themed roots for foreign targets — and changing how it is named and wired. Naming: one stem, `portalElement`, from `editor.registerPortalElement` and `mount(el, { portalElement })` through `resolvePortalElement`, `usePortalElement` and `PortalElementOverride` to the `portalElement` prop on every popover, menu and select. `portalRoot`, `portalTarget`, `portalContext` and `editorPortal` are gone. Where a forwarded prop and the ambient element coexist they are `portalElementProp` and `portalElement`. Theming: `ThemedRootProps` is replaced by a single `applyThemedRoot(element)` on `BlockNoteViewContext`, composed from the base classes and whatever the UI library adds. The mantine wrapper keeps base's `applyThemeVariables` ref for the editor container and passes the same function down for portal roots, so `BlockNoteTheme.ts` is untouched. Plumbing: the default portal element is derived in `usePortalElement` from the editor's own container, so `BlockNoteViewContainer` needs no state, no merged refs and no provider wrapper. `PortalElementOverride` creates its themed root directly and mounts it in a layout effect, replacing a `createPortal` of an empty div, a ref/state round-trip and a `closest(".bn-root")` read during render.
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/diagram-block
@blocknote/mantine
@blocknote/math-block
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
|
…aries Inside a component that receives a `portalElement` prop, `portalElement` now means that prop, and the surrounding default from `usePortalElement` takes the qualified name. Reads more directly, and keeps the props destructures on one line as they are on the base branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A rework of #3046, opened against
portals-contextso the diff is only the delta: 42 files, +503 / −389. (#3052 is the same branch opened againstmain, where the diff also contains the mobile-toolbar stack underneath.)It keeps #3046's model — portal elements registered on the editor, themed roots for foreign targets,
portalElementsreaching every UI element — and changes how it is named and wired.Rationale
Two things came out of reviewing #3046. The vocabulary had four stems for what are really two concepts — portal element / portal root / portal context / editor portal. And the mounting logic was doing more React work than it needed to, in a place where re-render and ordering bugs are expensive and hard to spot.
Changes
One vocabulary, two concepts.
portalElementeverywhere:PortalElement,portalElements,resolvePortalElement,usePortalElement,PortalElementOverride,editor.registerPortalElement/unregisterPortalElement,mount(el, { portalElement }), and theportalElementprop on every popover, menu and select (wasportalRoot). Where a forwarded prop and the ambient element coexist in one scope they areportalElementPropandportalElement..bn-root) becomes a single function rather than a props object:applyThemedRoot(element)onBlockNoteViewContext.ThemedRootPropsis removed.Theming goes back to where it was.
BlockNoteTheme.tsis restored byte-identical tomobile-toolbar-demo—applyBlockNoteCSSVariablesFromThemeandremoveBlockNoteCSSVariablesuntouched, including their set/unset behaviour. The mantine view keeps its existingref={applyThemeVariables}for the container and passes the same logic down asapplyThemedRootfor portal roots, replacing theuseEffectthat themededitor.portalElement. Net: base plus 12 lines, instead of a rewrite.Simpler plumbing.
usePortalElementderives the default from the editor's own container, soBlockNoteViewContainerreturns to its original shape — nouseState, no merged refs, no provider wrapper, nouseEditorDOMElementcoupling inBlockNoteView.PortalElementOverridecreates its themed root directly and mounts it in a layout effect: three plain effects (append, theme, register). That removes thecreatePortalof an empty div, the ref/state round-trip, theneedsContainerstate, and theclosest(".bn-root")DOM read during render.Impact
Behavioural improvement: the default portal element now resolves to the editor's
.bn-containerrather than the contenteditable'sparentElement. UnderrenderEditor={false}those differ — the app's own wrapper is the parent — so floating UI could otherwise be clipped by that wrapper'soverflow.Testing
Measured in browser mode (chromium):
bn-root light bn-mantine→bn-root dark bn-mantine,data-color-schemeanddata-mantine-color-schemeboth following a live toggle — the bug feat: Portals cleanup #3046 set out to fix, preserved..bn-containerin the plain case, in therenderEditor={false}+ wrapper case, and yields to an explicit override.null→ attached), against three on this base.mobile-toolbar-demorenders once but hands over a detached element, with no notification when it attaches.E2E chromium: 294 passed, 1 failed —
static.test.tsxtiming out at 90s. It passes standalone in 2.4s and in CI, which shards chromium into halves; it only times out when all 48 files share one local container.Not yet run: firefox/webkit, and the mobile end-to-end suite, which lives above this layer.
Open question
The default portal element costs consumers one extra render at mount. Making it an imperatively created div inside the container removes that (measured: one render, never null, attached before paint) at the cost of one empty DOM node — the node
mobile-toolbar-demoalready had. About 15 lines if we want that trade.