-
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
Add a zoomed out view to the site editor #41156
Conversation
Size Change: +1.23 kB (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
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 know this is still a draft, but I'm liking it. :) Left notes as I noticed things along the way.
packages/block-editor/src/components/block-tools/exploded-mode-inserters.js
Outdated
Show resolved
Hide resolved
I added some designs to the original issue some of which could be applicable here. |
packages/block-editor/src/components/block-list/use-block-props/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/use-block-props/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-tools/block-selection-button.js
Outdated
Show resolved
Hide resolved
speak( | ||
__( | ||
'You are currently in edit mode. To return to the navigation mode, press Escape.' | ||
) | ||
); | ||
} else if ( mode === 'exploded' ) { | ||
speak( __( 'You are currently in exploded mode.' ) ); |
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.
@jameskoster @critterverse we need to find a good name for this state
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.
Thanks for trying this out! I like how easy this makes it to add whole sections to the page. A slight issue I noticed in testing is that the in-between inserter tends to disappear after a block is moved (tested on Chrome/mac OS):
inserter_disappears.mp4
Another thing I'm noticing is that "Move to" in the block toolbar options is no longer working; on clicking "Move to" nothing happens except focus is transferred back to the block toolbar. It seems fine on trunk. I haven't looked through the code in-depth yet, but "Move to" works by setting navigation mode so may be something around the changes to how we're using isNavigationMode
in this PR.
@tellthemachines Thanks for the review
I'm not familiar with this feature personally, in my testing in both trunk and this branch, it works very weirdly if you're a mouse user. Or I should say, it doesn't work if you're a mouse user. It took me some time to figure out that it works using keyboard only and it seems to behave in the same way for me in both here and trunk. Maybe I'm missing something?
Yeah, I noticed this, there's a number of situations where these inserters don't seem to respond properly. I haven't been able to find the reasons for these yet. I'm considering only showing the inserters relative to the selected block to try to mitigate this but I'll try to figure out more. |
No, that's how it works. Ideally it should work with mouse too, but we never found a good way of doing that. As it is, it's the only way of moving blocks between nesting levels with keyboard only, so if we were to re-think it we'd have to keep that in mind. Yesterday I was testing with Gutenberg.run, but I now checked out this branch and can confirm it's working properly. Not sure what happened there 😅 |
There was a bug in one of the selectors I changed, I didn't fix it specifically for this issue but maybe it was a side effect there too. |
Side note on the "Move to" action, would it make sense to trigger one of the bottom left notifications when engaged explaining how it works? "Use the keyboard to position the block" |
I've improved I think the positioning update of the in-between inserters, it's almost fluid. We can do better maybe but it's acceptable I think. What do you all think is missing from this PR to consider it good enough for merge (v1)? |
fc28658
to
294bb84
Compare
Thinking about the broader UX I believe this zoomed out view could represent an opportunity to fuse the template editor with the site editor. I shared a concept in #27847 (comment) which illustrates how we can use it to toggle the visibility of the full layout while working on a post. It is also conceivable that it could be the default site editing view as well. However in both of those cases some of the features of this PR can be undesirable... I may still want to edit child blocks even while zoomed out to view the template, or I might want to concentrate on patterns while zoomed all the way in to my post. That's not to say that these features are not valuable, they are! But I think they might be more useful when you are zoomed further out (something that Matias alluded to in #39281 (comment)). If zoom is a feature of the site/template editor, then maybe these features come online at a certain zoom level, and perhaps this PR could represent phase one of that implementation? So to bring all this back to something actionable... I think it might be good to try zooming out even further, and to ideate on the entry-point a bit. I noticed it's still possible to select child blocks via List View. Should we adjust list view so that only root-level blocks are listed? One other small thing: The gap beneath the document is disproportionately larger than the one above when zoomed out. |
right: nextRect ? nextRect.left : previousRect.right, | ||
bottom: previousRect ? previousRect.bottom : nextRect.bottom, | ||
right: previousRect ? previousRect.right : nextRect.left, | ||
bottom: previousRect ? previousRect.left : nextRect.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.
Just double-checking — Is this line correct? By following the same logic of the rest of the return values, it should have been previousRect ? previousRect.top : nextRect.top
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.
(asking also because of https://github.com/WordPress/gutenberg/pull/43691/files#r964934557)
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.
Hm.. it seems so, yes.. I've tested with your suggested values though and couldn't see any difference. 🤔 .
I also tried to test in your PR but it doesn't contain this PR. Can you rebase to test there as well? Since you're changing the logic in your PR, do you think it's okay to handle this there? I could create a follow up of course for this..
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.
and couldn't see any difference. 🤔 .
That's expected, because passing height: 0
kind of overrides the bottom
value.
In #43691, I decided to pass an actual DOMRect
instead of a manually-created object — which should also remove any confusion. I would therefore ask you to take a closer look to the component while reviewing that PR 🙏
Noting that this is on my list to include in the next call for testing for the outreach program! Great work, folks. cc @ironprogrammer as test lead for 6.1 - this is good to have on your radar. |
Nice, Anne, I would love to find out if the way this mode is invoked feels naturally discoverable, and what it instinctually gets used for! We have ideas and goals of course, but input can help inform that 👌 |
Update: Originally reversed the plus/minus suggestion 🤦🏼♂️. Corrected below. This feature is smooth and feels very natural. A great addition for navigating template areas! 🎉 I would like to suggest using a different icon from As an alternative that is consistent with many other interface tools, how about using a So the icon in the zoomed-out/active state would look something like: Down the road, this may also set the stage for other zoom-like editor modes, such as "zooming in" another step for "Spotlight mode" to enter block isolation mode. *There is currently no |
I would agree that the chevrons aren't the best icon for it, but go a bit further and say: I'm not sure this button is the best way to invoke it. For this first version it was necessary to have a button, but I would love to see this mode integrated more seamlessly into a flow, rather than have a bespoke action on the toolbar. For example it might invoke by default as soon as you open the "pattern" tab in the inserter, or it could potentially be the default view for select mode. This is something to explore separately. |
Yeah, the invocation tool is mostly for testing, but it doesn't make sense there, we should make it contextual to the actions that demand it. |
When playing around with it - my instinct was that the zoomed out mode matched so naturally with thinking in patterns that I wanted them to work together that way...
|
Good note. Those buttons have zero gap and are 36x36, but should have 8px gap and be 32x32: As we make such a change, should we also consider making the 36px tall Publish/Settings buttons 32px height? With most buttons moving towards 40px height, the Publish button would be an exception and follow "toggle size" rules: What do you think, @WordPress/gutenberg-design ? |
I would love to unify the spacing in the top bar, it's all over the place right now. Any reason not to make buttons 40px? Then they align with other controls and are easier to tap on touch devices. |
Would need to see it, but since we use 32px for toggles elsewhere already, that size already feels well established. |
Not all of the buttons are toggles though – ellipsis, undo, redo, mode switcher. Either way I'm not sure it's intuitive to unilaterally make all toggles 32px 🤔 |
I can't picture 40px being a responsible use of that space, though, so I do like the idea that we have just two sizes, 32 and 40 to work with, both being base8, rather than what we have now which includes 36 in some places. |
I find it most important to focus on the usability aspect, and 32px touch targets are fairly small... iirc Material recommends a minimum of 44px. But yes, let's mock it all up and see! |
A mobile experience is crucial, though, but if the large sizes don't fit it's always an option to have desktop specific sizes as well. |
40px looks bananas for icon buttons 🙈 |
👋 a heads up that git bisect points to this commit as having introduced #44481. |
Any update or follow-up to the flow and icon issue? I see only #44585 but I'm not sure there's any issue to keep track of the icon issue. The icon is still there and it's terribly confusing: |
Related #39281 #40319
Alternative to #40376
What?
The idea of this PR is that the exploded view has proven to be very challenging and might not be worth it. So I'm just trying a zoomed out view instead like it's being discussed in #39281
See #39281 (comment)
The zoom-out view is the idea that on the site editor, you can enter a mode where the focus is more on site building and composing patterns, rather than editing granular blocks.
The current PR is the first step to that:
Testing instructions