Skip to content
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.

Sign up for GitHub

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

Simplify zoom out animation trigger #67533

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

Simplify zoom out animation trigger #67533

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
464b12f
Simplify zoom out animation trigger
ajlende Dec 3, 2024
8d93ba3
Re-add removing is-zoomed-out
ajlende Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 13 additions & 37 deletions packages/block-editor/src/components/iframe/use-scale-canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,6 @@ export function useScaleCanvas( {
const isZoomedOut = scale !== 1;
const prefersReducedMotion = useReducedMotion();
const isAutoScaled = scale === 'auto-scaled';
// Track if the animation should start when the useEffect runs.
const startAnimationRef = useRef( false );
// Track the animation so we know if we have an animation running,
// and can cancel it, reverse it, call a finish event, etc.
const animationRef = useRef( null );
Expand Down Expand Up @@ -271,7 +269,6 @@ export function useScaleCanvas( {
* - Sets the transitionFrom to the transitionTo state to be ready for the next animation.
*/
const finishZoomOutAnimation = useCallback( () => {
startAnimationRef.current = false;
animationRef.current = null;

// Add our final scale and frame size now that the animation is done.
Expand Down Expand Up @@ -309,40 +306,15 @@ export function useScaleCanvas( {

const previousIsZoomedOut = useRef( false );

/**
* Runs when zoom out mode is toggled, and sets the startAnimation flag
* so the animation will start when the next useEffect runs. We _only_
* want to animate when the zoom out mode is toggled, not when the scale
* changes due to the container resizing.
*/
useEffect( () => {
const trigger =
iframeDocument && previousIsZoomedOut.current !== isZoomedOut;

previousIsZoomedOut.current = isZoomedOut;

if ( ! trigger ) {
return;
}

startAnimationRef.current = true;

if ( ! isZoomedOut ) {
return;
}
Comment on lines -330 to -332
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


iframeDocument.documentElement.classList.add( 'is-zoomed-out' );
return () => {
iframeDocument.documentElement.classList.remove( 'is-zoomed-out' );
};
}, [ iframeDocument, isZoomedOut ] );

/**
* This handles:
* 1. Setting the correct scale and vars of the canvas when zoomed out
* 2. If zoom out mode has been toggled, runs the animation of zooming in/out
*/
useEffect( () => {
const startAnimation = previousIsZoomedOut.current !== isZoomedOut;
previousIsZoomedOut.current = isZoomedOut;

if ( ! iframeDocument ) {
return;
}
Expand All @@ -360,12 +332,12 @@ export function useScaleCanvas( {
} );
}

if ( scaleValue < 1 ) {
if ( isZoomedOut ) {
Comment on lines -363 to +335
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note the original conditional was scaleValue < 1 and now the conditional is scale !== 1.

Was that intentional?

Copy link
Contributor

@jeryj jeryj Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// If we are not going to animate the transition, set the scale and frame size directly.
// If we are animating, these values will be set when the animation is finished.
// Example: Opening sidebars that reduce the scale of the canvas, but we don't want to
// animate the transition.
if ( ! startAnimationRef.current ) {
if ( ! startAnimation ) {
iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-scale',
scaleValue
Expand All @@ -376,6 +348,8 @@ export function useScaleCanvas( {
);
}

iframeDocument.documentElement.classList.add( 'is-zoomed-out' );

iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-content-height',
`${ contentHeight }px`
Expand Down Expand Up @@ -407,10 +381,7 @@ export function useScaleCanvas( {
* - After the animation is complete, remove the fixed positioning
* and set the scroll position that keeps everything centered.
*/
if ( startAnimationRef.current ) {
// Don't allow a new transition to start again unless it was started by the zoom out mode changing.
startAnimationRef.current = false;

if ( startAnimation ) {
/**
* If we already have an animation running, reverse it.
*/
Expand Down Expand Up @@ -466,6 +437,10 @@ export function useScaleCanvas( {
}
}
}

return () => {
iframeDocument.documentElement.classList.remove( 'is-zoomed-out' );
};
}, [
startZoomOutAnimation,
finishZoomOutAnimation,
Expand All @@ -479,6 +454,7 @@ export function useScaleCanvas( {
containerHeight,
maxContainerWidth,
scaleContainerWidth,
isZoomedOut,
] );

return {
Expand Down
Loading