From 95712221841d646fe8aa414361fbd32f05234528 Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Tue, 29 Oct 2024 14:39:32 -0500 Subject: [PATCH] Only control zoom level in useZoomOut if starting from non-zoomed state This modifies the idea of the useZoomOut state to allow for a controlled mode. If the user begins in zoom out or manually toggles the zoom level, then we assume they are a user who understands the zoom levels and doesn't need this toggled for them. If they begin from a non-zoomed state, we can zoom in when they reach the patterns tab and remove the zoom level when they exit the inserter. If they use the zoom button when the inserter is open, this also removes the control mode, as they understand how to enter and exit zoom out. The idea here is: - it is jarring to have the zoom level automatically change for you, so default to not changing it unless it seems like the user doesn't know how to use the zoom button. - have a smart default, but allow overrides --- .../block-editor/src/hooks/use-zoom-out.js | 28 ++++++---- .../site-editor/site-editor-inserter.spec.js | 51 ++----------------- 2 files changed, 22 insertions(+), 57 deletions(-) diff --git a/packages/block-editor/src/hooks/use-zoom-out.js b/packages/block-editor/src/hooks/use-zoom-out.js index fd409c8465790b..85224ddee46196 100644 --- a/packages/block-editor/src/hooks/use-zoom-out.js +++ b/packages/block-editor/src/hooks/use-zoom-out.js @@ -22,15 +22,21 @@ export function useZoomOut( zoomOut = true ) { const { isZoomOut } = unlock( useSelect( blockEditorStore ) ); const toggleZoomOnUnmount = useRef( false ); - const userZoomState = useRef( null ); + const zoomStateOnMount = useRef( null ); const isZoomedOut = isZoomOut(); + const controlZoomLevel = useRef( null ); useEffect( () => { // Store the user's manually set zoom state on mount. - userZoomState.current = isZoomOut(); + zoomStateOnMount.current = isZoomOut(); + + // If they start in a zoomed out state, then do not control the zoom state. + // This user knows about the zoom levels and can toggle it when they want, + // or they have manually toggled it on and want to stay there. + controlZoomLevel.current = ! isZoomOut(); return () => { - if ( ! toggleZoomOnUnmount.current ) { + if ( ! controlZoomLevel.current || ! toggleZoomOnUnmount.current ) { return; } @@ -47,11 +53,12 @@ export function useZoomOut( zoomOut = true ) { * update it when isZoomedOut changes. */ useEffect( () => { - // Requested zoom and current zoom states are different, so toggle the state. - if ( zoomOut !== isZoomedOut ) { + // If we are in controlled mode and the requested zoom and current zoom states + // are different, toggle the zoom state. + if ( controlZoomLevel.current && zoomOut !== isZoomedOut ) { // If the requested zoomOut matches the user's manually set zoom state, // do not toggle the zoom level on unmount. - if ( userZoomState.current === zoomOut ) { + if ( zoomStateOnMount.current === zoomOut ) { toggleZoomOnUnmount.current = false; } else { toggleZoomOnUnmount.current = true; @@ -63,6 +70,7 @@ export function useZoomOut( zoomOut = true ) { setZoomLevel( 'auto-scaled' ); } } + // Intentionally excluding isZoomedOut so this hook will not recursively udpate. // eslint-disable-next-line react-hooks/exhaustive-deps }, [ zoomOut, setZoomLevel, resetZoomLevel ] ); @@ -73,11 +81,11 @@ export function useZoomOut( zoomOut = true ) { */ useEffect( () => { // If the zoom state changed (isZoomOut) and it does not match the requested zoom - // state (zoomOut), then it means the user manually changed the zoom state and we should - // not toggle the zoom level on unmount. + // state (zoomOut), then it means the user manually changed the zoom state while + // this hook was mounted, and we should no longer control the zoom state. if ( isZoomedOut !== zoomOut ) { - toggleZoomOnUnmount.current = false; - userZoomState.current = zoomOut; + // Turn off all automatic zooming control. + controlZoomLevel.current = false; } // Intentionally excluding zoomOut from the dependency array. We want to catch instances where diff --git a/test/e2e/specs/site-editor/site-editor-inserter.spec.js b/test/e2e/specs/site-editor/site-editor-inserter.spec.js index 459d8fadbe30b8..ad727eeb15e88b 100644 --- a/test/e2e/specs/site-editor/site-editor-inserter.spec.js +++ b/test/e2e/specs/site-editor/site-editor-inserter.spec.js @@ -119,7 +119,7 @@ test.describe( 'Site Editor Inserter', () => { await expect( await InserterUtils.getZoomCanvas() ).toBeHidden(); } ); - test( 'should return you to zoom out if starting from zoom out', async ( { + test( 'should not toggle zoom state between tabs if starting from zoom out', async ( { InserterUtils, } ) => { const zoomOutButton = InserterUtils.getZoomOutButton(); @@ -147,16 +147,14 @@ test.describe( 'Site Editor Inserter', () => { await blocksTab.click(); await expect( blocksTab ).toHaveAttribute( 'data-active-item', 'true' ); - // Zoom out should disengage - await expect( await InserterUtils.getZoomCanvas() ).toBeHidden(); + // Zoom out should still be egnaged + await expect( await InserterUtils.getZoomCanvas() ).toBeVisible(); // Close the inserter await inserterButton.click(); await expect( blockLibrary ).toBeHidden(); - // We should return to zoom out since the inserter was opened with - // zoom out engaged, and it was automatically disengaged when selecting - // the blocks tab + // We should stay in zoom out state since it was manually engaged from the start await expect( await InserterUtils.getZoomCanvas() ).toBeVisible(); } ); @@ -226,47 +224,6 @@ test.describe( 'Site Editor Inserter', () => { // We should stay in zoomed out state since it was manually engaged await expect( await InserterUtils.getZoomCanvas() ).toBeVisible(); } ); - - // Covers bug found in https://github.com/WordPress/gutenberg/pull/66381#issuecomment-2441540851 - test( 'should return to initial zoom out state after switching between multiple tabs', async ( { - InserterUtils, - } ) => { - const zoomOutButton = InserterUtils.getZoomOutButton(); - const inserterButton = InserterUtils.getInserterButton(); - const blockLibrary = InserterUtils.getBlockLibrary(); - - await zoomOutButton.click(); - await expect( await InserterUtils.getZoomCanvas() ).toBeVisible(); - - await inserterButton.click(); - const patternsTab = InserterUtils.getBlockLibraryTab( 'Patterns' ); - const blocksTab = InserterUtils.getBlockLibraryTab( 'Blocks' ); - const mediaTab = InserterUtils.getBlockLibraryTab( 'Media' ); - - // Should start with pattern tab selected in zoom out state - await expect( patternsTab ).toHaveAttribute( - 'data-active-item', - 'true' - ); - await expect( await InserterUtils.getZoomCanvas() ).toBeVisible(); - - // Go to blocks tab which should exit zoom out - await blocksTab.click(); - await expect( blocksTab ).toHaveAttribute( 'data-active-item', 'true' ); - await expect( await InserterUtils.getZoomCanvas() ).toBeHidden(); - - // Go to media tab which should enter zoom out again since that's the starting state - await mediaTab.click(); - await expect( mediaTab ).toHaveAttribute( 'data-active-item', 'true' ); - await expect( await InserterUtils.getZoomCanvas() ).toBeVisible(); - - // Close the inserter - await inserterButton.click(); - await expect( blockLibrary ).toBeHidden(); - - // We should re-enter zoomed out state since it was the starting point - await expect( await InserterUtils.getZoomCanvas() ).toBeVisible(); - } ); } ); class InserterUtils {