Decouple "zoom/scaling the canvas" from zoom out mode (without mode rename) #65482
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # packages/block-editor/src/components/block-tools/zoom-out-toolbar.js # packages/block-editor/src/components/tool-selector/index.js # packages/block-editor/src/store/reducer.js # packages/edit-site/src/components/global-styles/screen-style-variations.js # packages/editor/src/components/visual-editor/index.js
# Conflicts: # packages/block-editor/src/hooks/use-zoom-out.js
# Conflicts: # packages/editor/src/components/editor-interface/index.js
getdave
added
[Type] Code Quality
getdave
requested review from
draganescu,
youknowriad,
talldan and
MaggieCabrera
getdave
commented
Sep 19, 2024
Comment on lines
2096
to
2106
| 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.
Let's make the nomenclature consistent. Is it "zoom-out" or "zoom-level"? type and naming need to be consistent.
There was a problem hiding this comment.
Zoom out is anytime zoom level is less than 1.
There was a problem hiding this comment.
It's usually best to avoid using floating point numbers in JS. I went for 100 to mirror the traditional zoom scale of 0-100.
Is there a reason why you are suggesting 1? Is it used somewhere else already?
getdave
commented
Sep 19, 2024
Comment on lines
+570
to
+582
| 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; | ||
| } |
There was a problem hiding this comment.
Let's make the nomenclature consistent. Is it "zoom-out" or "zoom-level"? type and naming need to be consistent.
There was a problem hiding this comment.
I think this is all right, zoom out is any zoom level less than 1.
There was a problem hiding this comment.
See #65482 (comment)
getdave
commented
Sep 19, 2024
getdave
commented
Sep 19, 2024
Comment on lines
-13
to
+14
| * 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. |
There was a problem hiding this comment.
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.
Don't we need the hook for all the effect management it does?
There was a problem hiding this comment.
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.
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.
- Open Inserter
- Open Patterns Tab
- Select something on canvas
- Bug one: (it will be in the normal editing mode, not zoom out)
- Unset Zoom Out via header button
- Click Zoom Out button in header again
- Enters Zoom out for real
- Close Inserter
- Bug 2: In zoom out mode while on full canvas
Screen.Recording.2024-10-09.at.3.57.44.PM.mov
There was a problem hiding this comment.
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.
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.
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.
I didn't know that was a requirement -- that's helpful info for sure!
There was a problem hiding this comment.
Screen.Recording.2024-10-10.at.9.11.53.AM.mov
Is 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.
I agree that if we ever allow zooming to be separate from the mode or allow a slider to adjust zoom out,
Unfortunately in Gutenberg "next" (i.e. trunk) thats precisely what is being aimed for. So it felt correct to decouple the hook also.
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:
- is the hook a public API
- when was the hook introduced (which release)
- should the refactor be a "regression"?
- if so should we revert it entirely
getdave
changed the title
|
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. |
|
Size Change: +535 B (+0.03%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
draganescu
reviewed
Sep 19, 2024
There was a problem hiding this comment.
I think this is great! Nomenclature wise I think "zoom out" is fine as a syntactic sugar for zoom level less than 1. It is a bit confusing that the mode which affects editing and selection is also called zoom out but since that is likely to be deprecated maybe it's fine?
Bug In this PR the Global Styles > Browse styles is better than trunk in that selection is default but the canvas is zoomed out. However, clicking exits the zoomed out view.
Comment on lines
+570
to
+582
| 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; | ||
| } |
There was a problem hiding this comment.
I think this is all right, zoom out is any zoom level less than 1.
Comment on lines
-13
to
+14
| * 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. |
There was a problem hiding this comment.
Don't we need the hook for all the effect management it does?
Comment on lines
2096
to
2106
| 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.
Zoom out is anytime zoom level is less than 1.
This is a strange bug. I ruled out Update: one of the |
|
@draganescu I've normalised everything towards "level" for consistency and to disambiguate it from I also reduced some complexity where I was able. |
getdave
changed the title
draganescu
approved these changes
Sep 20, 2024
| useDispatch( blockEditorStore ) | ||
| ); | ||
|
|
||
| return useRefEffect( | ||
| ( node ) => { | ||
| if ( editorMode !== 'zoom-out' ) { | ||
| // In "compose" mode. | ||
| const composeMode = editorMode === 'zoom-out' && isZoomOut(); |
There was a problem hiding this comment.
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.
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 👍
getdave
added
[Package] Block editor
MaggieCabrera
approved these changes
Sep 20, 2024
noisysocks
added
the
Backport to WP 6.7 Beta/RC
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Sep 23, 2024
…ename) (#65482) * Use separate state for Zoom Level Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: MaggieCabrera <onemaggie@git.wordpress.org> # Conflicts: # packages/block-editor/src/components/block-tools/zoom-out-toolbar.js # packages/block-editor/src/components/tool-selector/index.js # packages/block-editor/src/store/reducer.js # packages/edit-site/src/components/global-styles/screen-style-variations.js # packages/editor/src/components/visual-editor/index.js * Refactor useZoomOut hook to use new state # Conflicts: # packages/block-editor/src/hooks/use-zoom-out.js * Use Zoom and Editor Mode in Toggle * Only hide footer when zoom level is set # Conflicts: # packages/editor/src/components/editor-interface/index.js * Adjust double click to exit zoom out mode * Standardise nomenclature * More standardisation of nomenclature * Reduce complexity of state and actions * Fix accidental breakage of Styles panel zoom * Fix hook * Reduce hook to single effect
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: c4b7319 |
97 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Decouples concept of "zooming the canvas" from "composing with patterns".
In this PR we retain
zoom-outas the mode name.Alternative to #65265
Why?
See #65265
How?
See #65265
Only difference is that we do not rename the mode - it remains
zoom-out.Testing Instructions
Same as #65265
Testing Instructions for Keyboard
Screenshots or screencast