refactor(react): one vocabulary and simpler plumbing for portal elements - #3052
Draft
YousefED wants to merge 11 commits into
Draft
refactor(react): one vocabulary and simpler plumbing for portal elements#3052YousefED wants to merge 11 commits into
YousefED wants to merge 11 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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 |
|
…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.
@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: |
BREAKING: `editor.mount(element, { portalTarget })` loses its options
argument. The option decided where `editor.portalElement` was appended, and
that element no longer exists; it had already been reduced to an alias for
`registerPortalElement`, which callers can call directly and explicitly:
editor.mount(element);
editor.registerPortalElement(someContainer);
Only needed when floating UI renders outside the editor's DOM tree — UI next
to the contenteditable already counts as within the editor.
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
2 participants
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 the portal consolidation in #3046. It keeps that PR'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.Important
This PR is opened against
main, so its diff contains the whole mobile-toolbar stack underneath it (51 commits, 178 files). The portal work itself is the single commit at the tip: 42 files, +503 / −389. To review only that, compare againstportals-context(#3046) — that is the intended base if this lands as a stacked PR.mobile-toolbar-demo(#2939)portals-context(#3046)Rationale
Review of #3046 raised two things. 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 subtle re-render and ordering bugs are expensive.
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) is now expressed as a single function rather than a props object:applyThemedRoot(element)onBlockNoteViewContext.ThemedRootPropsis removed.Theming stays where it was.
packages/mantine/src/BlockNoteTheme.tsis byte-identical tomobile-toolbar-demo—applyBlockNoteCSSVariablesFromThemeandremoveBlockNoteCSSVariablesare untouched. The mantine view keeps its existingref={applyThemeVariables}for the editor container and passes the same logic down asapplyThemedRootfor portal roots, replacing theuseEffectthat used to themeeditor.portalElement. It is base plus 12 lines.Simpler plumbing.
usePortalElementfrom the editor's own container, soBlockNoteViewContainergoes back to its original shape: nouseState, no merged refs, no provider wrapper.PortalElementOverridecreates its themed root directly and mounts it in a layout effect. That replaces acreatePortalof an empty div, a ref/state round-trip, aneedsContainerstate, and aclosest(".bn-root")DOM read during render.Impact
No public API is removed that has shipped —
editor.portalElementwas introduced after v0.54.0, so all of this is pre-release surface.One behavioural improvement over #3046: the default portal element 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 previously be clipped by a wrapper'soverflow.Testing
Verified in browser mode (chromium), all measured rather than asserted:
bn-root light bn-mantine→bn-root dark bn-mantine, withdata-color-schemeanddata-mantine-color-schemeboth following a live theme toggle. This is the bug feat: Portals cleanup #3046 set out to fix..bn-containerin the plain case, in therenderEditor={false}+ wrapper case, and yields to an explicit override.null→ attached element), against three on feat: Portals cleanup #3046.mobile-toolbar-demorenders once, but the element it hands over on that render is detached, with no notification when it attaches.E2E chromium: 294 passed, 1 failed. The failure is
static.test.tsxtiming out at 90s; it passes standalone in 2.4s and passes in CI, which shards chromium into halves — it only times out when all 48 files run in one local container.Not yet run: firefox/webkit, and the mobile end-to-end suite, which lives above this layer in the stack.
Breaking change
editor.mount(element, { portalTarget })loses its options argument. The option decided whereeditor.portalElementgot appended, and that element is removed by this work; it had already become a one-line alias forregisterPortalElement. Callers do it explicitly instead:Only needed when floating UI renders outside the editor's DOM tree — UI next to the contenteditable already counts as within the editor. No in-repo caller used it except one test; the docs never mentioned it.
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. Roughly a 15-line change if we want that trade.