WIP: Try to fix Popover obstructs blocks #43162
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
requested review from
Mamaduka,
mirka,
talldan,
ciampo and
ellatrix
|
Size Change: +544 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
Looks very promising in testing. I realise this is still a draft, but it'd be great to add more comments to the calculations to describe what each on does as that will make it easier to review. The only thing I'd consider as a nice addition to the current state of the PR is for the block to stay in place when scrolling if it's below the reference. Currently if the toolbar is below a block at the top of the canvas, when scrolling down it moves back to the top. |
67 tasks
ciampo
reviewed
Aug 12, 2022
There was a problem hiding this comment.
Thank you for working on this, Nik! The code in this PR is very interesting 👀
it'd be great to add more comments to the calculations to describe what each on does as that will make it easier to review
I have the same feedback — having comments would definitely help reviewing this PR.
For example, which bits are targeted at fixing this placement issue ?
Also tries to solve: Block Toolbar Regression - Toolbar obstructs selected blocks(#41575)
While this PR seems promising in terms of results, I would personally prefer a solution where we try to minimize the amount of custom middleware code in Popover and rely more on standard floating-ui features as much as possible — especially since it looks like the behavior that we're implementing here is quite fitter to the block toolbar.
The way this problem could be abstracted, in my mind, could be to:
- keep track, for each block, of the block's offset from the top of the page (and recalculate it only when necessary — i.e. not on page scroll)
- when the block toolbar is rendered, check if the selected block has a gap from the top of the page that can fit the toolbar (including the toolbar`s offset)
- if the gap isn't enough, turn on the
flipmiddleware on thePopover(via a prop that we should probably add)
(this, by the way, is very similar to the feedback that I left on #42887)
I've updated floating-ui to the latest version as I needed it for this change.
We should probably update the version of floating-ui anyway in a separate PR
| // If the reference has no height we don't need to do anything. | ||
| referenceRect[ mainDimension ] && | ||
| referenceRect[ mainAxis ] < 0 && | ||
| ownerDocument.documentElement.scrollTop <= |
There was a problem hiding this comment.
Just flagging that reading the scrollTop property could cause reflow / layout trashing, which could affect the page's performance (especially since this middleware is computed as the user scrolls and interacts with the page)
|
Agreed on closing this PR, although I'll keep it mind as a source of inspiration for future Popover work. Thank you @ntsekouras for you work here ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Partially based on #42950 for
Popover: fix shifting behavior to avoid popover overlapping its anchor. This PR attempts to also handle theleftplacement issue.
Also tries to solve:
Block Toolbar Regression - Toolbar obstructs selected blocks(#41575)I've updated floating-ui to the latest version as I needed it for this change.