feat: Portals cleanup - #3046
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks 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 |
| // Given a target element, checks whether a `.bn-root` element is somewhere up the DOM tree, as | ||
| // one is necessary to apply correct theming & styling. If one doesn't exist, creates one and | ||
| // returns it, both as a React node and HTML element. Otherwise, just returns the target element or | ||
| // null if the target is undefined. | ||
| function usePortalRoot(target: HTMLElement | undefined): { | ||
| root: HTMLElement | null; | ||
| themingContainer: ReactNode; | ||
| } { | ||
| const rootProps = useBlockNoteViewContext()?.portalRootProps; | ||
|
|
||
| const [needsContainer, setNeedsContainer] = useState<{ | ||
| target: HTMLElement; | ||
| value: boolean; | ||
| }>(); | ||
| const [containerElement, setContainerElement] = useState<HTMLElement | null>( | ||
| null, | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (target) { | ||
| setNeedsContainer({ target, value: !target.closest(".bn-root") }); | ||
| } | ||
| }, [target]); | ||
|
|
||
| if (!target || needsContainer?.target !== target) { | ||
| return { root: null, themingContainer: null }; | ||
| } | ||
|
|
||
| if (!needsContainer.value) { | ||
| return { root: target, themingContainer: null }; | ||
| } | ||
|
|
||
| return { | ||
| root: containerElement, | ||
| themingContainer: createPortal( | ||
| <div {...rootProps} ref={setContainerElement} />, | ||
| target, | ||
| ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
I didn't look at this deeply, but my first thought is whether we can just always have a container? I would rather not rapidly mount/unmount elements based on effects of what their parent tree looks like.
There was a problem hiding this comment.
We could, it would just clutter the DOM somewhat since we could get .bn-roots nested within other .bn-roots. Also for the record, the container element should only get mounted once
@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: |
| // Default to the ambient portal target (a themed `.bn-root`) so popovers | ||
| // inherit light/dark mode instead of the document body's, and escape the | ||
| // mobile formatting toolbar's horizontal scroll clip. | ||
| const contextPortal = usePortalContext(); |
There was a problem hiding this comment.
We need better naming for this. what's the difference between portalRoot and contextPortal? portalRoot also comes from a context 🤷♂️
There was a problem hiding this comment.
Updated naming, hopefully it's a bit clearer. Found it kinda difficult to have names that are not too long and also not too vague.
| () => resolvePortalTarget(portalElements?.default), | ||
| [portalElements?.default], | ||
| ) ?? | ||
| editorDOMElement?.parentElement ?? |
There was a problem hiding this comment.
if this defaults to editorDOMElement?.parentElement, I don't think portalTarget is a descriptive name for this
There was a problem hiding this comment.
I'm also curious if there are race conditions here or whether we can initialize things simpler.
useEditorDOMElement will be null initially, and then passed as <PortalTarget>. Than once things are mounted, it will be set to the bn-container (BlockNoteViewContainer)
There was a problem hiding this comment.
To avoid confusion - portalTarget is put in a React a context so that any popovers further down the render tree can use it to figure out where they need to portal to. So editorDOMElement?.parentElement is not special just because it's the editor container element, portalTarget is just saying to any popovers that they should portal there if they need to. In any case, hopefully things are clearer with the updated naming.
3 participants
Summary
This PR attempts to consolidate and clean up how portalling is handled in UI elements.
The trigger for this comes from the mobile formatting toolbar. It requires dropdowns to be portalled outside of it as it has
overflow: auto. On desktop,position: fixedelements like the dropdowns are not clipped by an ancestoroverflow: autoelement, but this behaviour varies between platforms on mobile. Additionally, the mobile toolbar itself needs to be portalled todocument.bodyas it should float on top of everything else on the page.Originally, a proprietary
MobileToolbarPortalContextwould set a target element for the dropdowns to portal to. This was clunky as there is already theBlockNoteViewportalElementsprop which is supposed to handle portalling for editor UI elements. Alongside that there'seditor.portalElement, which is set byportalElements.defaultand is used byisWithinEditorto check whether something is in the editor UI. However, this was also broken as it didn't take into account the other portals set byportalElements. The other portals were also broken as they don't receive proper styling/theming, as that relies on a container element with various attributes (things like a.bn-rootclass,data-themeattribute, etc). Overall, I think it's quite clear that there's a benefit to consolidating all of these things into one and fix some existing bugs along the way.So this PR does so by introducing
PortalTargetandusePortalContext, which replacesMobileToolbarPortalContextand is also howportalElementscreates portals. It takes atargetand exposes it in aPortalContextso any descendants always know where an element should be portalled to, by consuming saidPortalContext. AdditionalPortalTargets can also be added anywhere in the render tree to override the target element (useful especially for the mobile formatting toolbar where it and descendant dropdowns portal todocument.body). The reason we need aPortalTargetcomponent instead of just using a barePortalContext, is because the portalled elements need to be inside a container element for styling - same issue that already exists withportalElements. Therefore,PortalTargetfirst checks if a.bn-rootcontainer element exists up the DOM tree, and creates it if necessary.Rationale
isWithinEditorandportalElementshave bugs which partially due to this.editor.portalElementhas a pretty convoluted implementation and is treated as a special case.Changes
PortalTargetandusePortalContext.reactpackage to always usePortalTarget&usePortalContainerto set/get portal elements.editor.portalElementwitheditor._portalRoots. This is now a map which contains all created portals, whichisWithinEditorreads to guarantee that all editor-related elements are checked.Impact
N/A
Testing
TODO
Screenshots/Video
N/A
Checklist
Additional Notes
N/A