Open
Conversation
ajlende
added
[Type] Code Quality
|
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: -24 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
ajlende
force-pushed
the
fix/zoom-out-animation-trigger
branch
from
524c772 to
e6ff0fe
Compare
|
Flaky tests detected in e6ff0fe. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12144080449
|
ajlende
force-pushed
the
fix/zoom-out-animation-trigger
branch
from
e6ff0fe to
8d93ba3
Compare
getdave
reviewed
Dec 5, 2024
Comment on lines
-363
to
+335
| if ( scaleValue < 1 ) { | ||
| if ( isZoomedOut ) { |
Contributor
There was a problem hiding this comment.
I note the original conditional was scaleValue < 1 and now the conditional is scale !== 1.
Was that intentional?
Contributor
There was a problem hiding this comment.
tl;dr: I think it's fine to use isZoomedOut here.
We can't check scale < 1 directly because scale could be the string auto-scaled.
The intention for this line before was to check for isZoomedOut, but we didn't want the useEffect to run when that changed. So, we used scaleValue and didn't notice the checks were different. I think using isZoomedOut here is better than the check before since it will always be the same result instead of having two different checks for the same thing.
jeryj
reviewed
Dec 5, 2024
Contributor
jeryj
left a comment
jeryj
left a comment
There was a problem hiding this comment.
Working well! Glad this is finally combined into one useEffect! 👏🏻
Comment on lines
-363
to
+335
| if ( scaleValue < 1 ) { | ||
| if ( isZoomedOut ) { |
Contributor
There was a problem hiding this comment.
tl;dr: I think it's fine to use isZoomedOut here.
We can't check scale < 1 directly because scale could be the string auto-scaled.
The intention for this line before was to check for isZoomedOut, but we didn't want the useEffect to run when that changed. So, we used scaleValue and didn't notice the checks were different. I think using isZoomedOut here is better than the check before since it will always be the same result instead of having two different checks for the same thing.
Comment on lines
-330
to
-332
| if ( ! isZoomedOut ) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
I do like the early return to show that we're not doing anything if we're not zoomed out. Could we add this to the combined useEffect to make it clearer it doesn't do anything if we're not zoomed out?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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?
Simplifies the zoom out animation trigger.
Why?
Code quality.
How?
Since we're no longer relying on the
useEffectdependencies to trigger the animation, we can consolidate that effect into the main one.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast