-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: Vertical toolbar placement in Zoom Out mode #65370
Fix: Vertical toolbar placement in Zoom Out mode #65370
Conversation
- Watch target element for mutation - Add mutation observer
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @AhmarZaidi! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Thanks for the PR. Will look to review 👍 |
@WordPress/gutenberg-components you are probably well placed to advise on this PR. |
const targetElement = document.querySelector( | ||
'.interface-interface-skeleton__body' | ||
); |
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.
Usually, we shouldn't work directly with DOM. Have you considered using a ref instead?
|
||
// Observe mutations on the parent div of the inserter to update toolbar position. | ||
const targetElement = document.querySelector( | ||
'.interface-interface-skeleton__body' |
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.
The Popover
component is part of @wordpress/components
's API. interface-interface-skeleton__body
is declared within InterfaceSkeleton
, which is part of @wordpress/interface
.
However, @wordpress/components
does not know about @wordpress/interface
at all, nor it should care about the inserter, so tracking mutations of an unknown, unrelated to Popover
element doesn't make much sense.
If this is the right fix, it likely needs to be part of InterfaceSkeleton
's implementation, or the inserter, and not part of Popover
's internals.
I'm not convinced this is the right fix, anyway. This looks like something that should be resolved within floating-ui
, cc @ciampo and @jsnajdr who know its internals better than me.
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.
Yup, Marin is right. The @wordpress/components
package shouldn't be aware of the @wordpress/interface
package (since it's part of the Gutenberg app), and can't be tightly coupled to any of its references.
We should look for another way to fix this.
My gut feeling tells me that the bug is currently caused by popover anchor updates not being tracked correctly and used to re-render the popover.
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.
Hey @ciampo & @tyxla,
Thanks for the review!
I understand now that interface-interface-skeleton__body
is part of @wordpress/interface
and code from @wordpress/components
should not interact/depend on it.
The popover toolbar ref points to the currently selected block div, so that appears to be set correctly but as mentioned by @ciampo, the block div does not register layout changes when the inserter is toggled, thus it's not updating the position of toolbar.
Since, the inserter is added to the interface-interface-skeleton__body
, I added a mutation observer to it. It works but it's not the right solution. I have also tested putting layoutShift
to true but that doesn't seem to work.
layoutShift: false, |
Will explore more in the suggested direction to find another fix for this issue.
This comment was marked as resolved.
This comment was marked as resolved.
Should this get closed now that the vertical toolbar has been replaced with the default block toolbar? |
What?
This PR fixes the issue #65188
It updates the Popover component (used by the zoom out toolbar) to observe changes in the
.interface-interface-skeleton__body
element and trigger position updates accordingly.The
.interface-interface-skeleton__body
element is the parent div of the inserter.Why?
The current implementation does not update the popover toolbar's position when the inserter menu is opened in the zoom out mode. This PR ensures that the popover toolbar's position is updated correctly when layout changes occur to the parent inserter.
How?
MutationObserver
to observe changes in the parent div of the inserter.whileElementsMounted
function (which updates the toolbar's position relative to the reference element) to include the parent div of the inserter as a reference for useFloating.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before:
After: