-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Block Editor: Add block zooming. #16578
Conversation
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 generally agree with the usefulness of this functionality, but I wonder if we should call it zoom. Zoom may be associated with magnifying/scaling, while this feature does not necessarily make the selected blocks bigger, it just focus/crops the editor to show them. Maybe focus these blocks would be a way to describe the action?
@@ -1228,6 +1247,7 @@ export default combineReducers( { | |||
isTyping, | |||
isCaretWithinFormattedText, | |||
blockSelection, | |||
blockZoom, |
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.
If I create a group block, add some blocks inside it, select all the blocks inside, zoom them, and then press the undo button the editor crashes.
I guess blockZoom probably needs to be inside blocks reducer branch, so it is covered by undo functionality. As a side note, we should decide if zooming in creates a undo level or not, there are pros and cons of either option. But it is possible to have the functionality working correctly with undos without creating undo levels, like the block selection done in #16428.
Related #12327.
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.
Yes I need to move it, but I don't think it should create an undo level. It's not an edit operation, it's more like selections which we also don't expect to create undo levels.
I plan to give this reducer it's own history so that you can zoom out in steps after zooming in multiple times.
I wonder if it'll be easy to nest 2 reducers with history like that.
@@ -40,8 +40,8 @@ function useMovingAnimation( ref, isSelected, enableAnimation, triggerAnimationO | |||
ref.current.style.transform = 'none'; | |||
const destination = ref.current.getBoundingClientRect(); | |||
const newTransform = { | |||
x: previous ? previous.left - destination.left : 0, | |||
y: previous ? previous.top - destination.top : 0, | |||
x: previous ? previous.left - destination.left : -50, |
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.
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.
That's strange. I would think previous
is defined there. I'll look into it.
* | ||
* @return {Object} Updated state. | ||
*/ | ||
export function blockZoom( state, action ) { |
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.
Should the blockZoom reduce handle the case where a sibling of the zoomed blocks is inserted?
E.g: If add a paragraph, zoom it and then press enter the editor crashes.
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.
Yes, definitely.
switch ( action.type ) { | ||
case 'ZOOM_BLOCKS': | ||
return action.clientIds; | ||
case 'CLEAR_ZOOMED_BLOCKS': |
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.
Should we add cases for remove and replace block actions?
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.
Yes.
@jorgefilipecosta I like "focus" more too, but I thought there was some prior art to this. Here are the next steps:
|
Regarding this step |
Yeah, let’s get more eyes on it :)
…On Sun, Jul 14, 2019 at 10:37 AM Jorge Costa ***@***.***> wrote:
Regarding this step Change the term "zoom" to "focus"., feel free to
ignore it for now as it was just a thought and other people may think
differently.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#16578?email_source=notifications&email_token=AESFA2AMJQGIQAEJSDWK3GLP7NB3BA5CNFSM4IDB3XI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ4HZRY#issuecomment-511212743>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AESFA2GOP4356OK3K2WKG43P7NB3BANCNFSM4IDB3XIQ>
.
|
I would prefer "focus" to "zoom", since the latter makes me think of visually zooming into something, and I think that calling it "zoom" would cause many people to mistake it for something relating to accessibility. I'm not sure if "focus" is the best term, but I think it is better than "zoom". |
I started by trying to support insertions, replacements, and removals, but I quickly realized that it would take a pretty widespread refactor due to a lot of assumptions we make in the logic for getting insertion indexes and other data. I don't think the need for this feature warrants those changes. This makes me think that we might have to try a different approach for #16485 where the editor just switches between editing the template and editing post content with a static post title component like it does now.
That seems to do the trick.
I looked at the approach taken in #16428, but I didn't want to reimplement what was done there and just add another property to the new object that gets sent between the editor and the block editor. I also think that this need will start to pop up more and like #12327 describes, we'll need to sync any reference to blocks in history. This encompasses quite a bit of data and it would be better if we come up with some sort of generalized solution.
I'd say that focus is a term much more related to accessibility and that is actually my only concern with it. Focus is something applied to the currently edited field or selected button that gives accessibility tools' commands a target to operate on. I am not sure if it makes sense to reuse that term for what we are doing here. |
I agree with the others that "Zoom" doesn't really accurately describe this feature. "Focus" is closer, but there might be other ideas too. The main concern I have here is that I guess I'm not clear how things like — as in the example — background colors are handled. If you have a text block where you've applied a white color, but then zoom to the block, does there need to be some kind of accommodation to both (a) change the color to a visible color, and (b) prevent changing the color? By removing the context of the containing block, attributes which are dependent on that context could become a bit thorny. |
Looking at some other apps that have similar features Workflowy and Dynalist are hierarchical note taking apps. They have a feature they also both call 'zoom'. In Dynalist click the little magnifying glass next to a note (https://dynalist.io/demo) and in workflowy click the bulletpoint itself (https://workflowy.com/demo/embed/). Both use a little breadcrumb trail at the top to make it easy to zoom back out to the right place. Might be a consideration for this feature too. |
We did end up taking a different approach. I'll close this for now. |
@epiqueras Curious, was there any follow-up PR to this, or is this off the table for now? |
The new modes introduced in #16565 are sort of an evolution of this. |
Description
This PR implements a new block zooming feature that lets you focus the editor on the selected blocks through a button in the block toolbar. Furthermore, if one of the blocks selected has descendant blocks, the editor will unwrap them while it is zoomed.
A button on the top right of the block list lets you zoom back out and also serves as an indicator that the editor is zoomed.
This functionality will be crucial for maintaining the current user experience of editing a post after we move to a setup where post content itself is a block, like in #16485, or we introduce other top level blocks with complex descendant hierarchies. The idea is that users can zoom into a block like post content and edit the unwrapped descendants with the same ease of editing top level blocks. It lets them get around the clunkiness of editing inner blocks.
How has this been tested?
Both single and multi selections were zoomed into, with and without blocks with descendants, and the zooming worked as expected. The button to zoom back out also works as expected.
Screenshots
Types of Changes
New Feature: Add block zooming.
Checklist: