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
|
I want to understand this better, so @matthewlipski can we go over this at some point later? There is probably another approach here |
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.Additionally, it's been changed so that the height is no longer hardcoded, but instead the block content element's height is used. In some cases, the side menu is instead aligned to an element nested deeper inside, like for table blocks.
To streamline everything and make it more consistent, the side menu is now always centred vertically. Before, it would change depending on the block. E.g. it would be centred to the first line of paragraph and heading blocks, but aligned to the top of most file blocks, though not the audio block. This was done to make it more similar to Notion, but this resulted in a bunch of arbitrary exceptions having to be made.
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