Track if we programmatically changed zoom states for the user #66381
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. |
|
I'd like to add these manual tests as e2e tests. |
jeryj
added
[Feature] Zoom Out
[Type] Bug
|
Size Change: +71 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
|
Would this be targeting 6.7? You have a blocking review on #66330 (review) so I assume this will be the alternative? |
getdave
reviewed
Oct 24, 2024
There was a problem hiding this comment.
This all tests well to me.
The fundamental of this is that if you ever manually engaged Zoom Out and an action you took caused an automated behaviour to disengage Zoom Out (for UX reasons) then when you reverse that action the Zoom Out state will be restored.
The hook changes quite a bit between this and 6.7 since zoom out is no longer a mode. The 6.7 version of this hook has a similar but different method of tracking mode changes that is pretty reliable. You can still trick it (same with this one) with manually unsetting while in the mode, but it's minor IMO. |
getdave
approved these changes
Oct 25, 2024
|
|
||
| useEffect( () => { | ||
| const isZoomOutOnMount = isZoomOut(); | ||
|
|
||
| return () => { | ||
| if ( isZoomOutOnMount && isWideViewport ) { | ||
| // We never changed modes for them, so there is nothing to do. |
There was a problem hiding this comment.
Suggested change
| // We never changed modes for them, so there is nothing to do. | |
| // No mode change was made for the user, so no action is needed." |
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
jeryj
force-pushed
the
fix/use-zoom-out-track-change
branch
from
e2d6203
to
17ce681
Compare
draganescu
reviewed
Oct 28, 2024
| const isZoomedOut = isZoomOut(); | ||
|
|
||
| // Requested zoom and current zoom states are different, so toggle the state. | ||
| if ( zoomOut !== isZoomedOut ) { |
There was a problem hiding this comment.
This check is so confusing unless you scroll up to see the function signature 😆
There was a problem hiding this comment.
Agreed. Not sure what should be renamed. I think the passed zoomOut would be best to rename. Maybe change to setZoomState? Or zoomLevel?
draganescu
reviewed
Oct 28, 2024
| } | ||
| }, [ zoomOut, setZoomLevel, resetZoomLevel ] ); | ||
| }, [ manualIsZoomOutCheck ] ); | ||
| // Intentionally excluding zoomOut from the dependency array. We want to catch instances where |
There was a problem hiding this comment.
Maybe we should add a eslint exclude?
|
@jeryj testing this I still get another weird behavior:
This is a debatable "works as intended". But, if I do:
... it does not work as intented, since the manual zoom out toggle is overriden 🤷🏻 |
This is the expected behavior. Anytime we "guess" what the user wants we're going to have issues with things being weird. If they click to blocks, are they officially "done" with zoom out? Or did they not realize clicking to blocks would exit zoom out? I'll check on the other state you found. |
…e via blocks tab and back to zoom out tab
|
@draganescu I was able to fix the bug you found by also tracking the zoom state on mount, but it made the code less pretty :/ Basically, we're tracking if we should trust their zoom state on mount or not. I'd like to not need to track this too. The tests should majorly help in refactoring it at least. |
Maybe they wouldn't have been, but it's already done and working well, I think. There are e2e tests to catch regressions as well. That was the biggest reason this hook was so annoying in the past -- people kept accidentally breaking it. |
|
The problem of both this and https://github.com/WordPress/gutenberg/pull/66574is that they show the bug in global styles. I understand that neither cause the bug, but ... it'd be great if the one we merge also fixes the bug:
|
6 tasks
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.
Why?
Fixes a bug where zoom out was engaged/disengaged incorrectly on unmount.
How?
Adds a ref that tracks if we ever set a zoom level for them, and what we changed the zoom level to.
With this info, we can check on unmount for the correct states for if we should enter or exit zoom out on unmount (or do nothing).
Testing Instructions
From full screen mode
From full screen mode, exit zoom out manually
From zoom out, re-engage after unmount
Testing Instructions for Keyboard
Screenshots or screencast