Resets the Zoom on viewed/edited entity change #66232
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
getdave
added
[Type] Bug
getdave
requested review from
draganescu,
madhusudhand,
MaggieCabrera,
andrewserong and
aaronrobertshaw
getdave
added
the
Backport to WP 6.7 Beta/RC
|
Size Change: +67 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Nice one. Though I actually think we should be more aggressive. You should never see this state:
As soon as we exit to the site view—the above, with navigation on the left, preview frame on the right—zoom should disenge. The frame is meant to be just a preview, as soon as it's zoomed in with the gray material peeking in behind, it breaks that model. |
|
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. |
I've taken the liberty of pushing a commit with that change. (hope you don't mind me committing to your PR @getdave). |
|
Perfect thanks both 👍 |
Screen.Capture.on.2024-10-18.at.14-59-52.mp4I took the result for a spin. I think the animation is a bit rough and I think that's a problem because it's such a common action to open the "frame". I wonder if there's any way around that? |
kevin940726
approved these changes
Oct 20, 2024
Hmm, I'm not seeing that animation on either Gutenberg trunk or this PR branch; only on wp-develop. What I do see on Gutenberg trunk is a very broken editor when selecting a new template with zoom out mode enabled: zoomout_gutenberg.mp4Whereas this PR fixes that. On core the breakage isn't so obvious, but when switching to a new template while zoomed out, the new template doesn't appear zoomed out even though the button is in the selected state: zoomout_core.mp4I think disabling zoom out as soon as we exit a template makes sense and should fix the issue on core too. |
|
I've taken this for a spin. It seems to address the main issue for me 👍 ✅ Could replicate original issue While testing I noticed two minor issues while editing a page:
I couldn't replicate the same level of jankiness for the animation in the shared video but in my local env at the moment a lot of animations feel a little sluggish. As this PR does appear to fix the primary issue. Could we land it and iterate further to fix any animation issues or other inconsistencies? |
I was just in the middle of typing something to that effect. In my own testing I notice the jankiness appears a little differently in different browsers. I.e. subjectively Firefox seems a little smoother to me on my laptop vs Chrome. To me, looking into improving the animations is a slightly different problem to solve than this PR, which is an improvement over trunk, so my vote would be to land this, too. |
|
I opened #66268 to track the animation issue. Let's merge this and create follow-ups! |
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Oct 21, 2024
Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: tellthemachines <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: andrewserong <[email protected]> * Resets the Zoom on syncing to new entity * Zoom back out when exiting editor --------- Co-authored-by: Daniel Richards <[email protected]>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 0e34773 |
ellatrix
reviewed
Oct 24, 2024
|
|
||
| useEffect( () => { | ||
| if ( isReady ) { | ||
| __unstableSetEditorMode( 'edit' ); |
There was a problem hiding this comment.
Why can't we set an initial state instead? This is triggering a user preference to be set, which in turn triggers a REST API request to store this preference in the database, just by loading the site editor.
There was a problem hiding this comment.
Ok that's not good. Let's try and find a better fix.
There was a problem hiding this comment.
Ah...this doesn't affect WP 6.7 in the same way because it doesn't store the mode in a preference (only in state).
Nonetheless, this seems suboptimal.
There was a problem hiding this comment.
If I'm right we no longer want or need to set the editor mode here in trunk. If the mode is persisted as a preference then there's no need to reset it here. All we want to do is reset the zoom level.
In WP 6.7 the editor mode is relevant (and is only stored in state) so we can leave "as is".
There was a problem hiding this comment.
Candidate fix in #66452.
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
Co-authored-by: getdave <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: tellthemachines <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: andrewserong <[email protected]> * Resets the Zoom on syncing to new entity * Zoom back out when exiting editor --------- Co-authored-by: Daniel Richards <[email protected]>
youknowriad
reviewed
Nov 13, 2024
|
|
||
| useEffect( () => { | ||
| if ( isReady ) { | ||
| __unstableSetEditorMode( 'edit' ); | ||
| resetZoomLevel(); |
There was a problem hiding this comment.
Is this still needed? It feels like a very weird place for this action to be called (sync hook)
There was a problem hiding this comment.
I don't think the requirement to reset the zoom level when you move to a new entity has changed.
I struggled to find a suitable location for this. I assume the concern is that we're calling a UI-specific hook (i.e. zoom level) in a hook which is primarily concerned with initialisation of editor state?
Any more information on your concerns may help to refactor this.
There was a problem hiding this comment.
I actually removed this in a PR, so I could have introduced some regressions possibly. Based on your description, I would say we have two options:
- Potentially add this call to the before calling onNavigateEntity or something. (maybe a bit fragile)
- Potentially add this to
EditorProviderwhen the "post" prop (id, type) change.
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?
Follow up to #65932.
Resets the Zoom Out when moving to view/edit a new entity. Avoids Zoom Out remaining engaged when navigating between Pages/Templates...etc in the Site Editor.
Why?
Retaining the Zoom Out state when moving between entities can be confusing and may lead to bugs when previewing in the frame.
For example, moving between Pages in the Site Editor leaves Zoom Out intact. That's confusing because Zoom Out is feels like it's an action taken when editing a given entity. It shouldn't persist.
I appreciate this is fairly opinionated so I'm happy to defer to @WordPress/gutenberg-design if they don't feel this is the right route.
How?
Dispatches appropriate actions to reset Zoom when syncing entity with URL.
I elected not to put this in the
setEditedEntityaction because I didn't want to couple these two concepts too tightly.An alternative would be to reset Zoom Out if the Site Editor "frame" is opened (the black left hand sidebar with the
Wlogo).Testing Instructions
Pagesagain.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-10-18.at.11-58-13.mp4