-
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
Decouple "zoom/scaling the canvas" from zoom out mode (without mode rename) #65482
Changes from all commits
a37d291
0a1b36c
e026629
7b54277
167be6a
747a684
c443023
5b4f9a7
fff5db5
99008a4
c02a457
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,46 +8,40 @@ import { useEffect, useRef } from '@wordpress/element'; | |
* Internal dependencies | ||
*/ | ||
import { store as blockEditorStore } from '../store'; | ||
import { unlock } from '../lock-unlock'; | ||
|
||
/** | ||
* A hook used to set the editor mode to zoomed out mode, invoking the hook sets the mode. | ||
* A hook used to set the zoomed out view, invoking the hook sets the mode. | ||
Comment on lines
-13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This hook used to set the mode to Personally I think it should be deprecated as an API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we need the hook for all the effect management it does? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it could be useful for "zooming the canvas". But note how it's usage has already changed from managing "modes" to managing "scale". It's just a premature API in my opinion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file likely needs to be reverted to also handle the mode. It's being used to set the patterns tab to zoom out mode. By removing the mode handling, we're not setting the mode along with it, meaning you zoom out but aren't really in zoom out mode. This is causing some really odd states.
Screen.Recording.2024-10-09.at.3.57.44.PM.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Started a PR here to address it. Please take it over because I'm unsure what all the purpose was of your refactorings afterwards: #65999 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I understand why we needed this refactor at the moment. I agree that if we ever allow zooming to be separate from the mode or allow a slider to adjust zoom out, this would be necessary. But at the moment I'm not sure what we gain from this. Did it fix any specific bugs? I'm not trying to push back on the decision or be argumentative here -- I'm trying to understand what it gets us now vs a future potential. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't this fix the fact that we wanted global styles preview to only scale and keep the normal mode? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't know that was a requirement -- that's helpful info for sure! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Screen.Recording.2024-10-10.at.9.11.53.AM.movIs this what we want to happen on the styles tab though? I would assume not? It seems like it shouldn't have any editing mode attached to it? And if there is one, then zoom out would be more appropriate anyways? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Unfortunately in Gutenberg "next" (i.e. This was my concern about having a "fork" of the Zoom Out experience in 6.7 vs Gutenberg Plugin. I totally get where you're coming from @jeryj. Questions:
|
||
* | ||
* @param {boolean} zoomOut If we should enter into zoomOut mode or not | ||
* @param {boolean} zoomOut If we should zoom out or not. | ||
*/ | ||
export function useZoomOut( zoomOut = true ) { | ||
const { __unstableSetEditorMode } = useDispatch( blockEditorStore ); | ||
const { __unstableGetEditorMode } = useSelect( blockEditorStore ); | ||
const { setZoomLevel } = unlock( useDispatch( blockEditorStore ) ); | ||
const { isZoomOut } = unlock( useSelect( blockEditorStore ) ); | ||
|
||
const originalEditingModeRef = useRef( null ); | ||
const mode = __unstableGetEditorMode(); | ||
const originalIsZoomOutRef = useRef( null ); | ||
|
||
useEffect( () => { | ||
// Only set this on mount so we know what to return to when we unmount. | ||
if ( ! originalEditingModeRef.current ) { | ||
originalEditingModeRef.current = mode; | ||
if ( ! originalIsZoomOutRef.current ) { | ||
originalIsZoomOutRef.current = isZoomOut(); | ||
} | ||
|
||
return () => { | ||
// We need to use __unstableGetEditorMode() here and not `mode`, as mode may not update on unmount | ||
if ( | ||
__unstableGetEditorMode() === 'zoom-out' && | ||
__unstableGetEditorMode() !== originalEditingModeRef.current | ||
) { | ||
__unstableSetEditorMode( originalEditingModeRef.current ); | ||
} | ||
}; | ||
}, [] ); | ||
|
||
// The effect opens the zoom-out view if we want it open and it's not currently in zoom-out mode. | ||
useEffect( () => { | ||
if ( zoomOut && mode !== 'zoom-out' ) { | ||
__unstableSetEditorMode( 'zoom-out' ); | ||
// The effect opens the zoom-out view if we want it open and the canvas is not currently zoomed-out. | ||
if ( zoomOut && isZoomOut() === false ) { | ||
setZoomLevel( 50 ); | ||
} else if ( | ||
! zoomOut && | ||
__unstableGetEditorMode() === 'zoom-out' && | ||
originalEditingModeRef.current !== mode | ||
isZoomOut() && | ||
originalIsZoomOutRef.current !== isZoomOut() | ||
) { | ||
__unstableSetEditorMode( originalEditingModeRef.current ); | ||
setZoomLevel( originalIsZoomOutRef.current ? 50 : 100 ); | ||
} | ||
}, [ __unstableGetEditorMode, __unstableSetEditorMode, zoomOut ] ); // Mode is deliberately excluded from the dependencies so that the effect does not run when mode changes. | ||
|
||
return () => { | ||
if ( isZoomOut() && isZoomOut() !== originalIsZoomOutRef.current ) { | ||
setZoomLevel( originalIsZoomOutRef.current ? 50 : 100 ); | ||
} | ||
}; | ||
}, [ isZoomOut, setZoomLevel, zoomOut ] ); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -560,3 +560,23 @@ export function isZoomOutMode( state ) { | |
export function getSectionRootClientId( state ) { | ||
return state.settings?.[ sectionRootClientIdKey ]; | ||
} | ||
|
||
/** | ||
* Returns the zoom out state. | ||
* | ||
* @param {Object} state Global application state. | ||
* @return {boolean} The zoom out state. | ||
*/ | ||
export function getZoomLevel( state ) { | ||
return state.zoomLevel; | ||
} | ||
|
||
/** | ||
* Returns whether the editor is considered zoomed out. | ||
* | ||
* @param {Object} state Global application state. | ||
* @return {boolean} Whether the editor is zoomed. | ||
*/ | ||
export function isZoomOut( state ) { | ||
return getZoomLevel( state ) < 100; | ||
} | ||
Comment on lines
+570
to
+582
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make the nomenclature consistent. Is it "zoom-out" or "zoom-level"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is all right, zoom out is any zoom level less than 1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #65482 (comment) |
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.
What are we exiting, the view or the selection mode? I guess view, so it doesn't matter the mode, only if
isZoomOut
. Right?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 is one where I didn't know the answer. I kept the current behaviour on trunk which is that double click is connected to the mode.
As you say, let's merge and iterate 👍