-
-
Notifications
You must be signed in to change notification settings - Fork 660
fix: Suggestion menu positioning #2232
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
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: |
| Object.assign(elements.floating.style, { | ||
| maxHeight: `${availableHeight - 10}px`, | ||
| }); | ||
| apply(p) { |
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'm not sure about this, maybe @nperez0111 can double check when he's back
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.
Hm, yea I don't like this fix either. Will get back to it to see what we should do here. I think we may just need it to be re-evaluated once the underlying number of items changes
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.
Alright, I see what @matthewlipski was trying to do here. I think that I would prefer not to keep the fix as it feels like more of a hack than needed.
What is happening here is that the autoplacement middleware (which is very similar to the flip middleware) is trying to find the best spot to put the menu, and is unaware on the first render how large the element actually is, because it has not yet requested the items since they can be async.
To fix this, we would need to introduce something which would allow the child to request that the parent re-calculate it's position when the number of items changes. Like a context of our own for popovers or https://floating-ui.com/docs/FloatingTree
I think we can look into doing this at a later time though, because while the UX is not ideal, I'd prefer correctness in the code we introduce.
e2d8ffd to
e394108
Compare
Summary
This PR fixes an issue where if the suggestion menu was displayed above the trigger character (when there was not enough space to display it below), and only showed a few items, there would be a large gap between the menu and trigger character. This was because the suggestion menu had a hard-coded minimum height, which wasn't filled when displaying a small number of items.
Also, the positioning has generally been improved to better take advantage of the available screen space.
Rationale
This issue has made the suggestion menu feel janky in some situations.
Changes
SuggestionMenuControllerFloating UI optionssizemiddleware.flipmiddleware withautoPlacement. The difference is thatautoPlacementalways displays the menu on the side that has more space, whereasfliponly changes the side when there is not enough available space for the menu.Impact
N/A
Testing
Updated Notion UI testing doc.
Screenshots/Video
N/A
Checklist
Additional Notes