feat: Remove useRegisterPortalRoot from ShadCN - #3054
Conversation
|
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 |
|
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: |
YousefED
left a comment
YousefED
left a comment
There was a problem hiding this comment.
If this works it's SO much cleaner. Love both improvements!!
| trapFocus={portalRoot ? false : undefined} | ||
| // Do not move focus to the dropdown when requested (mobile), as it blurs | ||
| // the editor's contentEditable and dismisses the on-screen keyboard. | ||
| trapFocus={preventFocusOnOpen ? false : undefined} |
There was a problem hiding this comment.
nice improvement, so much better to have explicit naming like this
| const container = portalRoot ?? editorPortalElement ?? undefined; | ||
| // The `portalRoot` supplied at the call site is a themed `.bn-root`, so the | ||
| // menu inherits light/dark mode instead of the document body's. | ||
| const container = useContext(PortalRootContext) ?? undefined; |
There was a problem hiding this comment.
double check; but all scenarios keep working like this? because it feels like one option has been dropped here compared to container = portalRoot ?? editorPortalElement ?? undefined;
| // Portal the tooltip into the ambient portal target (a themed `.bn-root`) | ||
| // so it inherits the editor's light/dark color scheme instead of the | ||
| // document body's. | ||
| const editorPortalElement = useEditorPortalElement(); |
There was a problem hiding this comment.
there's still a dependency on useEditorPortalElement here. can you check all occurences?
There was a problem hiding this comment.
tried, but was cumbersome for Toolbar and Badges. Let's keep these exceptions for now (added two comments in code)
| @@ -48,6 +48,13 @@ type ToolbarSelectType = { | |||
| }[]; | |||
| isDisabled?: boolean; | |||
| portalRoot?: HTMLElement | null; | |||
There was a problem hiding this comment.
It seems like the pattern is now that we always pass portalRoot. In that case, maybe let's make this non-optional so we can validate the codebase does this correctly?
`portalRoot` is now required on `Menu.Root`, `Popover.Root` and the toolbar select. The UI-library adapters never read the editor's context, so they cannot fall back to it themselves; making the prop required lets the compiler enforce what was a convention. Call sites pass the ambient element as-is — `null` before the editor has mounted — instead of coercing it to `undefined`. What each adapter does with `null` follows its library, so none of them portal to the document body: - shadcn passes it through: Base UI waits for a container. - ariakit toggles `portal` off: it renders inline until there is one (Ariakit appends a fresh div to the body for null and undefined alike). - mantine already rendered inline on a falsy value. The shadcn tooltips (`ToolbarButton`, `Badge`) keep reading the ambient element through `useEditorPortalElement`. That is the one documented exception: mantine and ariakit tooltips render inline and would ignore a passed element, so a prop would buy nothing there.
`vp run build` typechecks example projects that lint doesn't cover: the three `SettingsSelect` copies render a `Toolbar.Select` without the now required prop. They sit inside `BlockNoteView`, so they read the ambient element and pass it. Also drops the last `?? undefined` on a portal value: the shadcn `Badge` tooltip passes the element as-is, like `ToolbarButton` already does, so nothing falls back to the body.
2 participants
Summary
Generally, the components in the
ariakit,mantine, andshadcnpackages that are used for theComponentsContextare supposed to be "dumb" adapters for primitives like buttons, menus, etc. In #3046, this contract was broken somewhat as ShadCN components required the use ofuseEditorPortalElement. This is because unlike the other 2 UI libs, which render dropdowns as siblings of their trigger elements, ShadCN portals them tobodyby default. This doesn't work for us as that puts them outside container elements which provide additional theming and styling (basically the same issue #3046 addresses). Therefore, the default portal location was overridden to the return ofuseEditorPortalElement.To ensure consistency across UI libs then, this PR makes it so that the
portalRootprop, which tells an Ariakit/Mantine/ShadCN component if & where it should portal its dropdown to, is always passed the element returned byuseEditorPortalElement.Additionally, the
preventFocusOnOpenprop has been added. This is so that for mobile, we can disable dropdowns from grabbing focus from the editor on open, as this would close the virtual keyboard. But on desktop, this should be enabled for better accessibility. This was previously set based on ifportalRootwas defined, which isn't quite right. Yes, it works in our case because the only time we'd actually setportalRootwas for the mobile formatting toolbar. But there is no direct correlation between a dropdown needing to be portalled, and preventing focus from moving to a dropdown on open, so these 2 concerns have been separated.Rationale
See above.
Changes
See above.
Impact
N/A
Testing
E2E snapshots will need updates, but I'm holding off on this until we decide whether this change is smth we want.
Screenshots/Video
Checklist
Additional Notes