-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
base: trunk
Are you sure you want to change the base?
Changes from 5 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 |
---|---|---|
|
@@ -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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2085,6 +2085,25 @@ export function hoveredBlockClientId( state = false, action ) { | |
return state; | ||
} | ||
|
||
/** | ||
* Reducer setting zoom out state. | ||
* | ||
* @param {boolean} state Current state. | ||
* @param {Object} action Dispatched action. | ||
* | ||
* @return {boolean} Updated state. | ||
*/ | ||
export function zoomLevel( state = 100, action ) { | ||
switch ( action.type ) { | ||
case 'SET_ZOOM_OUT': | ||
return action.zoom; | ||
case 'RESET_ZOOM': | ||
return 100; | ||
} | ||
|
||
return state; | ||
} | ||
|
||
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. Zoom out is anytime zoom level is 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. It's usually best to avoid using floating point numbers in JS. I went for Is there a reason why you are suggesting |
||
const combinedReducers = combineReducers( { | ||
blocks, | ||
isDragging, | ||
|
@@ -2118,6 +2137,7 @@ const combinedReducers = combineReducers( { | |
openedBlockSettingsMenu, | ||
registeredInserterMediaCategories, | ||
hoveredBlockClientId, | ||
zoomLevel, | ||
} ); | ||
|
||
function withAutomaticChangeReset( reducer ) { | ||
|
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 hook used to set the mode to
zoom-out
. Now it just sets the zoom level. Is that what we want? It's only used in a single location in Core.Personally I think it should be deprecated as an API.
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.
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 comment
The 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.