-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix scaling animation for device previews #66132
Conversation
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. |
// Handles transitions between device previews | ||
transition: all 400ms cubic-bezier(0.46, 0.03, 0.52, 0.96); | ||
@include reduce-motion("transition"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be re-added as a mixin or sass variable since it's used in multiple places again.
For reference, this is what we had before in base-styles/_animations.scss
.
@mixin editor-canvas-resize-animation() {
transition: all 0.4s cubic-bezier(0.46, 0.03, 0.52, 0.96);
@include reduce-motion("transition");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We overwrite it though, as we need to add more conditions to the end. I could having the mixin for just the timing. But if we add that mixin again, we'd still have to repeat the all 0.4s cubic-bezier(0.46, 0.03, 0.52, 0.96)
part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, if we added the mixin back, the other place that uses that code would look like:
@include editor-canvas-resize-animation;
$zoomOutAnimation: all 400ms cubic-bezier(0.46, 0.03, 0.52, 0.96);
// overwrite the mixin because we need to use it here.
transition: $zoomOutAnimation, transform 0s translate 0s scale 0s;
@include reduce-motion("transition");
&.zoom-out-animation {
// we only want to animate the scaling when entering zoom out. When sidebars
// are toggled, the resizing of the iframe handles scaling the canvas as well,
// and the doubled animations cause very odd animations.
transition: $zoomOutAnimation, transform 0s;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add additional rules to the mixin.
Size Change: +73 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
@@ -30,7 +26,7 @@ | |||
$outer-container-width: var(--wp-block-editor-iframe-zoom-out-outer-container-width); | |||
$container-width: var(--wp-block-editor-iframe-zoom-out-container-width, 100vw); | |||
// Apply an X translation to center the scaled content within the available space. | |||
transform: translateX(calc(( #{$outer-container-width} - #{ $container-width }) / 2 / #{$scale})); | |||
transform: translateX(calc((#{$outer-container-width} - #{$container-width}) / 2 / #{$scale})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments for how to simplify the mixin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Animations are back. Looks good to me. 👍
fixed-device-preview-animations.mp4
There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch. PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk.
|
Co-authored-by: jeryj <[email protected]> Co-authored-by: ajlende <[email protected]>
@jeryj Looks like this PR didn't pick to 6.7 cleanly. Would you be able to take a look? |
Co-authored-by: jeryj <[email protected]> Co-authored-by: ajlende <[email protected]>
Co-authored-by: jeryj <[email protected]> Co-authored-by: getdave <[email protected]>
Co-authored-by: jeryj <[email protected]> Co-authored-by: ajlende <[email protected]>
What?
Fixes the scaling animation between device previews because I was wrong. 😅
Why?
Device previews should have a scaling animation.
How?
Adds the animation back in. I'm not adding the mixin again, as they need to be handled slightly differently anyways.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast