chore: Move side menu height setting to FloatingUI #2224
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@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.
see comments.
Also it made me think of a separate thing; did we test the block-side menu in multi-column? afaik it wasn't listed under the test cases
| return { | ||
| element: node, | ||
| }; | ||
| const blockContentElement = blockElement.firstElementChild; |
There was a problem hiding this comment.
I'm not sure I like the use of firstElementChild. It assumes a specific dom structure to get the dom element - which is not typesafe / futureproof.
Better approach would be to use the methods we have like getblockinfo to get the content block, and then call getdomatpos for it
(example; the current approach would probably break for column blocks which don't have blockContent)
| middleware: [ | ||
| size({ | ||
| apply({ elements }) { | ||
| // TODO: Need to fetch the block from extension, else it's |
There was a problem hiding this comment.
this code is called in useMemo, so the apply function only gets recreated when the deps array changes
|
|
||
| if (block.type === "heading") { | ||
| if (!block.props.level || block.props.level === 1) { | ||
| elements.floating.style.setProperty("height", "78px"); |
There was a problem hiding this comment.
we can't base this on pixels here. What if someone changes the font or font-size in css?
Summary
This PR makes it so that the height of the side menu is now set in FloatingUI middleware on
SideMenuController. This is a change from the old method, which uses a more convoluteduseEffecthook + CSS.The side menu height is set in order to make it appear vertically centred for some blocks, and aligned top for others. While this is also possible with the
placementoption inuseFloatingProps, it's not exactly that simple due to e.g. file blocks with captions. For these, we want the side menu ignore the caption for the vertical alignment, so it's actually centred relative to the block.Additionally, two bugs with the file panel and side menu have been fixed. The file panel would show up below any nested blocks, while the side menu was horizontally indented with nested blocks. These were fixed by adding more options to the
BlockPopovercomponent, detailed in the changes list below.Rationale
This continues with the theme of moving responsibilities to FloatingUI where possible, which we started doing as part of the major refactor in #2143. The bugs are also regressions that should be fixed.
Changes
useEffecthook fromSideMenucomponent.sizemiddleware toSideMenuControllerto set the side menu height.includeNestedBlocksoption toBlockPopover. Whentrue, this works the same as before, using the entire block as the reference element. Whenfalse, it uses the block's content as the reference element. Defaults totrue.spanEditorWidthoption toBlockPopover. Whenfalse, this works the same as before, where theboundingClientRectis the same size & position as the reference element. Whentrue, thexposition and width are used for theboundingClientRect, and only the reference elementyposition and height are used.Impact
This actually also fixes a previously unknown bug with the file panel thanks to that addition of
includeNestedBlocks. Because it used the whole block as the reference element, it would show beneath all child blocks.Testing
Went through the Notion UI testing doc to ensure no regressions for UI elements using the
BlockPopover(file panel, side menu & AI menu). Added test cases to all UI elements for nested blocks.Screenshots/Video
N/A
Checklist
Additional Notes