[WIP] Account for positioned elements when positioning Block Toolbar #40625
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
getdave
added
[Feature] UI Components
|
Size Change: +359 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
scruffian
force-pushed
the
try/improve-block-toolbar-positioning
branch
from
a535887
to
0d939c5
Compare
|
When I try to add a child link to a navigation block I get a WSoD and this error: |
|
Fixed in ea40171 |
|
This looks very promising. On mobile we position the block toolbar at the top regardless of anything: An alternative to consider would be doing that with the navigation block. |
That was one of the suggestions in #36977 (comment) and we could explore it although I think it might be very strange with it suddenly jumping up to the top. |
Yeah, I think this approach is better |
|
Let's wait on #40740 to land before we continue here. |
|
This is so cool! 👏🏻 |
|
#40740 is merged, should we try to get this in? |
This is not a simple challenge. We will need to
From previous knowledge I believe |
Open
1 task
|
@scruffian @draganescu Do we want to pursue this now we have alternative means of editing Navigations "off canvas"? |
|
Given that we also have the top toolbar setting I think maybe we can close this one now. |
|
We may close the PR but keep the issue open as we still have a problem with rendering the floating block toolbar on top of block inner blocks. The toolbar should render at the edge of the bounding box of the block including its renders children. |
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?
This PR is an experiment which seeks the element which
Why?
In #40382 we learnt that some blocks have content with extends beyond the boundary of the block's root element.
As the block's toolbar is positioned immediately below the bottom boundary of the block, this can, on occasion, obstruct the ability to interact with crucial elements that are underneath the block toolbar.
This is most prominent on the Navigation block, where the toolbar will often be positioned on top of flyout submenus.
Closes #40382
How?
This PR explores one of the possible approaches outlined in #36977 (comment), namely:
Currently the implementation
Element.getBoundingClientRect().bottom<Popover>component.The implementation also watches for DOM mutations and will retrigger the toolbar positioning in case of changes (for example elements which are shown/hidden based on classnames .etc).
Caveats/Concerns
My main concern is inefficiency of the algorithm:
MutationObserverand I have a hunch that this could be computationally expensive.getComputedStyleis used which can force layout reflowsI also think this needs much more stress testing. What other 3rd party blocks have instances whether the block's content extends beyond its layout bounds?
Testing Instructions
Screenshots or screencast
Screen.Capture.on.2022-04-26.at.15-39-16.mov