-
-
Notifications
You must be signed in to change notification settings - Fork 459
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
fix: Scrolling responsiveness & UI element overflow #250
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Trying to understand why this works;
The theory is that Tippy calls getReferenceRect() at the "right" moment, and that previously the data that we return to getReferenceRect can be outdated?
To understand it:
- what are the scenarios where the data passed was outdated?
- at which moments does tippy call
getReferenceRect
that fixes the issues?
Are we only talking about onScroll
? and does Tippy attach it's own listeners to window?
packages/core/src/extensions/HyperlinkToolbar/HyperlinkToolbarPlugin.ts
Outdated
Show resolved
Hide resolved
packages/react/src/ElementFactory/components/ReactElementFactory.tsx
Outdated
Show resolved
Hide resolved
I think the issue wasn't necessarily due to outdated data, moreso that the entire component was being re-rendered on scroll. This would explain why you couldn't scroll down to see it if it was overflowing (since each scroll event would render a new component) and ofc why it wasn't as responsive. I'm not 100% sure how Tippy's |
packages/react/src/ElementFactory/components/EditorElementComponentWrapper.tsx
Show resolved
Hide resolved
// leaving time for the fade out animation to complete. | ||
// Used when hiding the element. If we were to pass in undefined instead, the | ||
// element would be immediately cleared, not leaving time for the fade out | ||
// animation to complete. |
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.
do we still need prevDynamicParams
even if the getBoundingclientRect
is now always available?
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.
Yeah we do, at least for the side menu and slash menus, since they need dynamic params to render as they fade out. Otherwise the side menu throws an error while the drag handle menu is open, and the slash menu renders with no items. Maybe these could be cached like the menu position is, though I feel like that's a bit beyond the scope of this PR.
Right now, every time we scroll, each
uiElement
is re-rendered. In general, I do think we want to leave the responsibility of figuring out when to render/hideuiElement
s completely up to BlockNote, Tippy has it's own listeners to figure out when it should update its position. Trying to re-render the whole component on scroll instead of letting Tippy sort it out itself leads to some issues, namely overflow handling and bad responsiveness.https://discord.com/channels/928190961455087667/1015169282444894219/1120228729600348181
https://discord.com/channels/928190961455087667/1015169282444894219/1120355999828680795