-
-
Notifications
You must be signed in to change notification settings - Fork 677
fix: Suggestion menu overflow issue #2363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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: |
|
Fix looks like it works to me! |
|
I added a max height of 600px - I think for this PR though, an overall UX fix-up is out of scope |
nperez0111
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @YousefED take one last look at this, since he knows best the edge cases. But this feels right to me
|
Great! The UX is a lot better. One minor bug is the page scroll shifts when navigating with keyboard (hold an arrow key to cycle through the list completely). Not sure if that's an easy fix? Regarding the new |
|
The page scroll shift bug looks like it's unrelated to the changes in this PR (also reproducible on website & main), so I think it's out of scope. I'll make a new issue for it though. As for the |
Makes sense!
I had a quick look, I think it does get triggered, but maybe we need to configure it differently? I found this guide, but didn't immediately found a fix. A different direction I'm thinking of would be to always set a fixed height of 600(?) and measure based of that (or use a virtual element to return 600). That would also solve a requirement that imo we don't want the menu to flip once the users filters and less options get available -> menu resizes -> menu flips (imo it's better to have a stable side). Can you experiment a bit further based of this? I agree there are quite some edge cases involved! |
|
I think I think that the real problem here is that the suggestion menu items are being rendered asynchronously no? Like why are we doing all of these workarounds for something that is actually synchronous most of the time? Even if not that, I think that autoUpdate is just an abstraction over doing the same sort of thing that we are doing here anyway no? It will just call update to re-calc and re-layout, so I don't understand why we are putting any more effort here if this solution is sufficient? |
We're passing a real element, so autoupdate should work
The Suggestion Menus are designed to be Async compatible, because users should be able to reuse the menu for things like user mentions (which might require an API call)
Two reasons top of mind:
|

Summary
This PR fixes an issue when on initial show, the suggestion menu would not correctly flip vertically if there was no space, and would only flip once the items updated.
This would happen because the
GenericPopoverresponsible for positioning and sizing the suggestion menu would do so based on its initial size. However, the menu fetches and renders its items async, so its initial size is far smaller than when it has all of its items.To fix this, the
GenericPopover's position is re-calculated when the suggestion menu items are updated.Rationale
The suggestion menu issue is pretty annoying for users and causes bad UX.
Changes
GenericPopoverUpdateContextwhich contains a function to update the position of the nearestGenericPopover.SuggestionMenuWrapperupdate ancestorGenericPopoverposition when after items are updated.autoPlacementmiddleware toflip(functions same as before, butautoPlacementwas inconsistent after this change).SuggestionMenuController.Impact
N/A
Testing
N/A
Screenshots/Video
N/A
Checklist
Additional Notes