Fix useZoomOut inserter behavior #67591
Merged
+333
−43
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
|
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. |
jeryj
added
[Feature] Zoom Out
[Type] Bug
|
Size Change: +71 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Dec 4, 2024
This was referenced Dec 4, 2024
Closed
|
Experience wise, this feels nice. It stays zoomed out when expected to, and zooms back in when appropriate. CleanShot.2024-12-05.at.11.59.23.mp4 |
richtabor
approved these changes
Dec 5, 2024
Mamaduka
reviewed
Dec 5, 2024
Mamaduka
reviewed
Dec 5, 2024
jeryj
force-pushed
the
fix/use-zoom-out-inserter-behavior
branch
from
1b32f30 to
dd36078
Compare
jeryj
force-pushed
the
fix/use-zoom-out-inserter-behavior
branch
from
dd36078 to
bb69b54
Compare
jeryj
force-pushed
the
fix/use-zoom-out-inserter-behavior
branch
from
bb69b54 to
3318d08
Compare
|
The same e2e test keeps failing even after rebasing and trying several times to rerun it. It passes locally. Not sure what's going on with it. It doesn't seem related to this PR. |
Merged
This test kept failing for me, and I isolated it to if a test before it had run await admin.visitSiteEditor(), then the preload test would fail because the user had previously visited the site editor. By resetting preferences before the test runs, it allows a cleaner enviroment and the test has consistent results.
jeryj
commented
Dec 6, 2024
| @@ -6,6 +6,7 @@ const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' ); | |||
| test.describe( 'Preload', () => { | |||
| test.beforeAll( async ( { requestUtils } ) => { | |||
| await requestUtils.activateTheme( 'emptytheme' ); | |||
| await requestUtils.resetPreferences(); | |||
There was a problem hiding this comment.
This should really be in a different PR, but I wanted to see if adding this would fix the e2e issue I'm running into. More context in the PR that implemented the Preload test.
tl;dr: The Preload test will have different results depending on what runs before it, and this should help normalize it. I just happened to run into the issue by having my test run on the same shard.
There was a problem hiding this comment.
It did work -- I would have liked this fix a different PR but it auto-merged once the tests passed.
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.
Fixes #66328
What?
Track if the useZoomOut hook changed the zoom level for the user. If we never changed it, don't change the zoom level on unmount.
Request from @richtabor: If we did change zoom level via the
useZoomOuthook (controlled mode) alwaysresetZoomLevel()on unmount.Why?
Fixes a bug where zoom out was engaged/disengaged incorrectly on unmount.
How?
Adds a
controlZoomLevelRefthat tracks if we ever changed the zoom level for the user. When unmounting, if we changed the zoom level, reset it unless they manually toggled the zoom level after us. In that condition,controlZoomLevelRefis set tofalse.Testing Instructions
The key concepts here are:
So, with these in mind, we have two scenarios to watch for:
From full screen mode
From full screen mode, exit zoom out manually
Screen Recording
Screen.Recording.2024-12-05.at.9.13.47.AM.mov