Disable Zoom Out if no section root to allow for Theme opt in #67232
+127
−16
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
[Type] Bug
getdave
requested review from
draganescu,
talldan,
jeryj,
richtabor,
MaggieCabrera,
ajlende and
annezazu
|
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: +145 B (+0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
jeryj
reviewed
Nov 22, 2024
|
Something like this? https://github.com/WordPress/gutenberg/pull/67245/files |
|
@jeryj I replied on your PR. |
|
Thanks for the feedback folks. If this is a route (forgive the pun) we want to go down then we could look into creating an editor level abstraction for this. However, in the interests of expediency I think we should check for the section root and see whether this PR resolves the problem. This is particularly true when the absence of section root is what is causing problems in Themes. @draganescu @jeryj Did you get a chance to test the PR to see whether it works? 🙏 |
ramonjd
reviewed
Nov 28, 2024
There was a problem hiding this comment.
Bit of a drive by test 👋🏻
This is working as described for me:
Trunk
Kapture.2024-11-28.at.11.37.43.mp4
This PR
Kapture.2024-11-28.at.11.39.55.mp4
I also checked scenarios that trigger zoom out mode in trunk. Opening the patterns inserter doesn't open zoom out mode, but Styles > Browse styles still does.
Personally, I think that's okay since, arguably, it still allows one to observe style changes to the entire page at once.
Anyway, this LGTM but another sanity check would help given my lack of context.
Thank you!
|
The E2E fails seem related, though I haven't confirmed locally |
getdave
force-pushed
the
fix/disable-zoom-out-if-no-section-root
branch
from
b4aad6f to
36c367e
Compare
Yes. Ironically it was because the tests for Zoom Out didn't include a section root! |
getdave
added
the
Backport to Gutenberg RC
|
I think we'll need to add these disable checks to the hooks as well because of course we sometimes enter Zoom Out programatically (e.g. when selecting the Will make some adjustments... |
getdave
force-pushed
the
fix/disable-zoom-out-if-no-section-root
branch
from
bab6c18 to
3d6b933
Compare
|
@ramonjd If you get a chance to take another look at this that would be great. I'd like to get this over the line so it can ship in the next Gutenberg release 🙏 |
This was referenced Nov 28, 2024
ramonjd
approved these changes
Nov 29, 2024
|
Test failures are legit again. Need to investigate. |
|
I've realised that this PR will disable Zoom Out mode in the Post Editor entirely. That's because there is no section root in that Editor. Personally I feel that's probably ok, but I'd be curious to see what others think. |
getdave
force-pushed
the
fix/disable-zoom-out-if-no-section-root
branch
from
b3cd9e4 to
dc26c49
Compare
This one is consistently failing. Trying to replicate manually and so far I cannot. |
ramonjd
reviewed
Dec 1, 2024
|
Thanks for helping with the tests @talldan - they had me a bit stumped yesterday. Admittedly, I'm easily stumped. |
This was referenced Dec 3, 2024
priethor
pushed a commit
that referenced
this pull request
Dec 4, 2024
* Disable toolbar and auto inserter behaviour if no section root * Remove useless coercion Co-authored-by: Ramon <[email protected]> * Remove more coercion copy/paste Co-authored-by: Ramon <[email protected]> * Add section root to Zoom Out e2e test * Add test coverage * Add some test coverage * Try e2e test fix by reverting all template part changes in Theme * Remove need to exit Zoom Out * Ensure a main tag * Update tests to expect the click-through behavior * Simplify selection --------- Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: draganescu <[email protected]>
|
Cherry-picked for the 19.8 release in c86612b ✅ |
priethor
removed
the
Backport to Gutenberg RC
im3dabasia
pushed a commit
to im3dabasia/gutenberg
that referenced
this pull request
Dec 4, 2024
…ess#67232) * Disable toolbar and auto inserter behaviour if no section root * Remove useless coercion Co-authored-by: Ramon <[email protected]> * Remove more coercion copy/paste Co-authored-by: Ramon <[email protected]> * Add section root to Zoom Out e2e test * Add test coverage * Add some test coverage * Try e2e test fix by reverting all template part changes in Theme * Remove need to exit Zoom Out * Ensure a main tag * Update tests to expect the click-through behavior * Simplify selection --------- Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: draganescu <[email protected]>
michalczaplinski
pushed a commit
that referenced
this pull request
Dec 5, 2024
* Disable toolbar and auto inserter behaviour if no section root * Remove useless coercion Co-authored-by: Ramon <[email protected]> * Remove more coercion copy/paste Co-authored-by: Ramon <[email protected]> * Add section root to Zoom Out e2e test * Add test coverage * Add some test coverage * Try e2e test fix by reverting all template part changes in Theme * Remove need to exit Zoom Out * Ensure a main tag * Update tests to expect the click-through behavior * Simplify selection --------- Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: draganescu <[email protected]>
getdave
added
the
Backport to WP Minor Release
|
Added the label in the hope of getting this backported to WP Minor release at some point. |
|
|
|
@getdave Also, ignore me. I didn't have my glasses on 😇 WP not Gutenberg. As you were. |
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?
Disable Zoom Out if no section root to allow for Theme opt in by defining a
<main>Closes #65400
Why?
Because reports have been coming in that the block layout of some Themes isn't compatible with a good experience in Zoom Out.
How?
With this change if a
<main>tag is not defined (or we're not editing a entity with acore/post-contentblock) then Zoom Out will be disabled.This allows Themes to opt in to Zoom Out based on where they place a
<main>tag. If no<main>is defined then Zoom Out will not work.Testing Instructions
<main>from templateTesting Instructions for Keyboard
Screenshots or screencast