Make zoom out vertical toolbar consistent #65627
Merged
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
draganescu
added
[Type] Bug
|
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. |
|
Size Change: +179 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ciampo
reviewed
Sep 26, 2024
draganescu
force-pushed
the
fix/consistent-zoom-out-toolbars
branch
from
bc8dea0
to
7c4075d
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Flaky tests detected in 9cc9dd2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11099593244
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
draganescu
added
the
Backport to WP 6.7 Beta/RC
draganescu
force-pushed
the
fix/consistent-zoom-out-toolbars
branch
from
5d5b959
to
f85357c
Compare
jsnajdr
reviewed
Oct 1, 2024
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
Co-authored-by: Dave Smith <[email protected]>
Co-authored-by: Dave Smith <[email protected]>
…cal toolbar, also fix typo
draganescu
force-pushed
the
fix/consistent-zoom-out-toolbars
branch
from
3b5b062
to
0afb084
Compare
… of dom classname
I discovered that there's a prop |
getdave
approved these changes
Oct 15, 2024
There was a problem hiding this comment.
For me this now tests really well and I can no longer find edge cases.
Great work here @draganescu for all the upfront effort and also @talldan for jumping in with improvements to selecting "the canvas".
I'd be happy to merge.
|
There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch. PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk. |
|
@draganescu Would you be able to try getting this to cherry pick cleanly into the |
draganescu
added a commit
that referenced
this pull request
Oct 15, 2024
* enable vertical toolbar for non full width elements, anchor based on parent * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Dave Smith <[email protected]> * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Dave Smith <[email protected]> * make zoom out check a dependency of the memoization, improve code readability * comment typos * subscribe to state instead of calculating zoom out view state when calculating the anchor * get the section wrapper for anchoring instead of the parent * use a selector instead of computing on the fly the parent section * check if the block element exists yet before computing the anchor * check if the block element exists yet before computing the anchor * differentiate between section toolbar and block toolbar for correct positioning when both are visible * address some nits * make the select in anchor setting rerun when block selection changes * fix bug with anchor rect when zoom out not engaged * fix typo * Use root container element in post editor as popover anchor * improve comment * improve comment to max improvement possible Co-authored-by: Dave Smith <[email protected]> * mega nit commit Co-authored-by: Dave Smith <[email protected]> * Fix bug with Posts with no full width blocks * give up on section root, always seek canvas element to position vertical toolbar, also fix typo * introduce the concept of canvas via a 1st variable * Use `__unstableContentRef` for zoomed out toolbar positioning instead of dom classname --------- Co-authored-by: draganescu <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: jsnajdr <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: andrewserong <[email protected]>
|
The cherry pick PR #66119 solved conflicts but now has a missing function. |
youknowriad
reviewed
Oct 15, 2024
| selectedElement, | ||
| popoverDimensionsRecomputeCounter, | ||
| isSectionSelected, | ||
| __unstableContentRef, |
There was a problem hiding this comment.
I don't really understand the change here. Can you explain why the block popover component needs to be aware of zoom-out...
There was a problem hiding this comment.
The reason I'm confused is because for me the "BlockPopover" is just a component that gives you the "position" of the block. So I don't see how zoom-out is related. It's not about giving the position of the toolbar, it's about giving the position of the block. It just happens that toolbar is adjacent to the block (in most cases). I think if we need to adapt the position differently in zoom-out, we should either have clear props in this component to change the behavior (for instance a prop that clearly indicates how we want to change the behavior) or do this computation elsewhere.
There was a problem hiding this comment.
"BlockPopover" is just a component that gives you the "position" of the block.
I don't know what to say about that, since this does render a whole popover, it's not some utility called to get the position. I do get the concern though.
I think if we need to adapt the position differently in zoom-out, we should either have clear props in this component to change the behavior
That is a good alternative sure. But at some point someone will have to "know" about zoom out.
Let's wait on the faith of this toolbar and if it sticks around return to reflect in code the idea that toolbars "just happen" to be adjacent to blocks. This was intended as a bug fix and it does not introduce a big glaring problem IMO just some minor tech debt reflecting the issue with multiple toolbar types.
Do you agree?
cbravobernal
pushed a commit
that referenced
this pull request
Oct 17, 2024
* enable vertical toolbar for non full width elements, anchor based on parent * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Dave Smith <[email protected]> * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Dave Smith <[email protected]> * make zoom out check a dependency of the memoization, improve code readability * comment typos * subscribe to state instead of calculating zoom out view state when calculating the anchor * get the section wrapper for anchoring instead of the parent * use a selector instead of computing on the fly the parent section * check if the block element exists yet before computing the anchor * check if the block element exists yet before computing the anchor * differentiate between section toolbar and block toolbar for correct positioning when both are visible * address some nits * make the select in anchor setting rerun when block selection changes * fix bug with anchor rect when zoom out not engaged * fix typo * Use root container element in post editor as popover anchor * improve comment * improve comment to max improvement possible Co-authored-by: Dave Smith <[email protected]> * mega nit commit Co-authored-by: Dave Smith <[email protected]> * Fix bug with Posts with no full width blocks * give up on section root, always seek canvas element to position vertical toolbar, also fix typo * introduce the concept of canvas via a 1st variable * Use `__unstableContentRef` for zoomed out toolbar positioning instead of dom classname --------- Co-authored-by: draganescu <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: jsnajdr <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: andrewserong <[email protected]>
cbravobernal
pushed a commit
that referenced
this pull request
Oct 17, 2024
* Make zoom out vertical toolbar consistent (#65627) * enable vertical toolbar for non full width elements, anchor based on parent * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Dave Smith <[email protected]> * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Dave Smith <[email protected]> * make zoom out check a dependency of the memoization, improve code readability * comment typos * subscribe to state instead of calculating zoom out view state when calculating the anchor * get the section wrapper for anchoring instead of the parent * use a selector instead of computing on the fly the parent section * check if the block element exists yet before computing the anchor * check if the block element exists yet before computing the anchor * differentiate between section toolbar and block toolbar for correct positioning when both are visible * address some nits * make the select in anchor setting rerun when block selection changes * fix bug with anchor rect when zoom out not engaged * fix typo * Use root container element in post editor as popover anchor * improve comment * improve comment to max improvement possible Co-authored-by: Dave Smith <[email protected]> * mega nit commit Co-authored-by: Dave Smith <[email protected]> * Fix bug with Posts with no full width blocks * give up on section root, always seek canvas element to position vertical toolbar, also fix typo * introduce the concept of canvas via a 1st variable * Use `__unstableContentRef` for zoomed out toolbar positioning instead of dom classname --------- Co-authored-by: draganescu <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: jsnajdr <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: andrewserong <[email protected]> * bring over two private selectors from trunk the fix depends on --------- Co-authored-by: Dave Smith <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: jsnajdr <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: andrewserong <[email protected]>
getdave
added
Backported to WP Core
|
Updating labels as backported in #66119 |
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
* enable vertical toolbar for non full width elements, anchor based on parent * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Dave Smith <[email protected]> * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Dave Smith <[email protected]> * make zoom out check a dependency of the memoization, improve code readability * comment typos * subscribe to state instead of calculating zoom out view state when calculating the anchor * get the section wrapper for anchoring instead of the parent * use a selector instead of computing on the fly the parent section * check if the block element exists yet before computing the anchor * check if the block element exists yet before computing the anchor * differentiate between section toolbar and block toolbar for correct positioning when both are visible * address some nits * make the select in anchor setting rerun when block selection changes * fix bug with anchor rect when zoom out not engaged * fix typo * Use root container element in post editor as popover anchor * improve comment * improve comment to max improvement possible Co-authored-by: Dave Smith <[email protected]> * mega nit commit Co-authored-by: Dave Smith <[email protected]> * Fix bug with Posts with no full width blocks * give up on section root, always seek canvas element to position vertical toolbar, also fix typo * introduce the concept of canvas via a 1st variable * Use `__unstableContentRef` for zoomed out toolbar positioning instead of dom classname --------- Co-authored-by: draganescu <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: jsnajdr <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: andrewserong <[email protected]>
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?
Closes #65785
Fixes #65570
Make sure all the elements identified as sections - full width or not.- display the same vertical toolbar specific to sections.
Why?
Consistent UI.
How?
ZoomOutToolbarfor all sections and remove the full width condition.Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
consistent-toolbar.mp4
Related #63519 - the difference in how in the post editor we only have an html container but no container block is problematic here too.
Co-authored-by: draganescu [email protected]
Co-authored-by: getdave [email protected]
Co-authored-by: talldan [email protected]
Co-authored-by: ciampo [email protected]
Co-authored-by: jsnajdr [email protected]
Co-authored-by: MaggieCabrera [email protected]
Co-authored-by: richtabor [email protected]
Co-authored-by: stokesman [email protected]
Co-authored-by: andrewserong [email protected]