Decouple "Zooming" from "Composing with Patterns/Sections" #65265
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
getdave
added
[Type] Experimental
getdave
requested review from
draganescu,
scruffian,
talldan,
jeryj,
MaggieCabrera and
ajlende
|
Size Change: +851 B (+0.05%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
ajlende
reviewed
Sep 12, 2024
|
Interesting. I'm not convinced on a separate "mode" for composing, but I am curious about the interactions accounting for #65204. An issue here is that zoom out behaves differently if I zoom out with the control, or engage "Compose" mode. I think zoom out interactions should be consistent, regardless of how it's engaged. If in #65204 we could select patterns, as well as content blocks, could we invoke that "Edit" mode? Where I can select a pattern to see the vertical toolbar, move it/delete it/etc, but also click any content parts on the page to edit. Perhaps that's something in the middle worth trying? |
These are different things and to me consistency considerations don't apply: zoom out is a view (it's a preset of scaled canvas) whereas the modality of working only with patterns / sections for composing is something completely different. That means zoom out as a view should be compatible in the future with other modes/workflows.
Basically just make is so that edit mode in #65204 also has the UX in this PR for "compose"? That reduces the diea that edit is for editing as b/c of the applied scaling the content is smaller. Sometimes the text could get too small. It'd be nice if in the event of combining compose and edit to incorporate the vertical toolbar in the horzontal toolbar and not need the scaling? |
|
Let's "hide" the mode from the toolbar and make the mode an unstable API (e.g. What do you think? |
Do we need to do that if |
|
+1 to removing from the toolbar |
getdave
force-pushed
the
try/decouple-zoom-out-compose
branch
from
6c88a0c
to
5acebac
Compare
getdave
commented
Sep 17, 2024
Comment on lines
+686
to
687
| ( editorMode === 'edit' || editorMode === 'compose' ) // Don't recalculate the initialPosition when toggling in/out of zoom-out mode | ||
| ? getSelectedBlocksInitialCaretPosition() |
There was a problem hiding this comment.
Not sure how to resolve this one
|
So what we should be trying here is this:
Then once #65204 is clearer either enhance compose to also have content only editing of sections, or replace it entirely with edit mode. In that case when the editor is in edit mode and the scale pref is not default we should enable the vertical toolbar. Composing with sections and simple editing are kinda overlapping but it is certain that the "effect" of zooming has to be a thing that is completely separate from editing or selection modes. |
getdave
commented
Sep 18, 2024
| typeof __experimentalSetIsInserterOpened === 'function' | ||
| ) { | ||
| __experimentalSetIsInserterOpened( false ); | ||
| if ( getEditorMode() === 'compose' ) { |
There was a problem hiding this comment.
Do we also want to check for being zoomed out here?
There was a problem hiding this comment.
If you are zoomed out but not in "compose" mode do we want double click to zoom in to work?
getdave
changed the title
getdave
added
[Type] Code Quality
The concept of "Zoom" is established so I don't see huge value in renaming to "scale". At least as part of this PR which is already large enough. We could address the nomenclature of "zoom" in a follow up if required.
The mode in this PR is already "compose" and "zoom-out" is removed 👍 The only traces of "zooming" that will remain reference the scaling of the editor canvas which is legitimate.
This is what the button does currently in the PR 👍 |
|
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. |
getdave
commented
Sep 18, 2024
| const ICON_MAPPING = { | ||
| edit: editIcon, | ||
| navigation: selectIcon, | ||
| compose: composeIcon, |
There was a problem hiding this comment.
Do we want to show a special icon for compose or just alias to the "edit" icon?
getdave
commented
Sep 18, 2024
| * | ||
| * @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 ) { |
There was a problem hiding this comment.
For me this is an example of a premature abstraction. A public API was created for a "mode" which was not yet considered stable (as indicated by the fact that the selector/action for the modes are __unstable.
We now have to maintain this API but make it work with the new concept which is not ideal.
I don't know the answer. My instinct is that we should make the mode private because we don't want to commit to it and the I've experimented with an approach for making |
That sounds good, I like the idea |
|
The whole discusson about private APIs has made me nervous. This Therefore I think we should
This will simplify the PR and allow us to move forward. |
Could it be deprecated and call through to whatever is replacing it? |
|
I've tried an alternative approach in #65482 which doesn't change the mode name. |
|
Closing in favour of #65482 |
2 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 "Zooming the canvas" from "Composing with Patterns".
zoom-outas a mode and replaces withcompose.Related #65482
Why?
Working on Zoom Out, I've noticed that sometimes we may want to "zoom out" on the canvas without going into a mode which prioritises "Composing with Sections".
For example, if you click
Styles -> Browse Stylesthe canvas will "zoom out". But in this mode you might not want to start composing your page. There may be other examples already or in the future.Also the term "Zoom Out" does not accurately describe the new mode. We are no longer just "zooming out on the canvas". Rather we are switching to a mode which prioritises "Composing a page using Patterns as Sections". The fact that the canvas is "zoomed" is really a byproduct.
How?
composeTesting Instructions
General
trunk- there should be no discernible difference in functionality.Browse Styles Toggle
Browse Stylescomposemode is not activeBrowse StylesUI reverts to the original zoom scale.Browse Styles- the Zoom level should be unaffected as you open/close that UI. The original Zoom should be preserved.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-09-11.at.20-06-31.mp4
Props
Co-authored-by: getdave get_dave@git.wordpress.org
Co-authored-by: ajlende ajlende@git.wordpress.org
Co-authored-by: richtabor richtabor@git.wordpress.org
Co-authored-by: draganescu andraganescu@git.wordpress.org
Co-authored-by: MaggieCabrera onemaggie@git.wordpress.org