Zoom Out: Remove zoom-out toolbar #66039
Merged
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
youknowriad
added
[Type] Bug
youknowriad
requested review from
draganescu,
jeryj,
richtabor,
MaggieCabrera,
ellatrix and
stokesman
|
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: -893 B (-0.05%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Thanks for raising this. It's worth considering. Are you proposing this for WordPress 6.7? Technically this isn't a bug and thus we shouldn't really be exploring it unless the Design Lead wants to make that call. Also cc'ing @richtabor whose designs originally proposed the vertical toolbar. |
|
I think this is a bug fix (it closes two bug fix issues). It's indeed a different way to fix the bugs though. So I would consider this for 6.7 assuming we all agree it's the right path. |
|
In #65592 I also tried to do away with some of the parts of the zoom out view that seemed ad-hoc and unweaved with the rest of the editor experience. This PR tries to do away with the section toolbar, which is an addition completely disconnected from the rest of the block interaction baseline - immediate acctions toolbar, extended actions inspector. We now get in zoom out a more or less useless inspector and a toolbar that replaces the normal toolbar kinda purely for aesthetic reasons. But! The aesthetic reasons have a reason to appear - generally our floating UI obscures content. This is less important at scale 1 but at scale 0.7 it becomes even more obvious. Basically the normal toolbar gets in the way of the most basic benefit of zooming out. However it may be that our bigger problem is that we still have not elegantly solved deselection, in an universal way. Zoom out does have a deselection mechanism via clicking on the workspace area (the gray background) but the zoomed in does not. Maybe we should preserve the workspace all all times? |
17 tasks
This comment was marked as off-topic.
This comment was marked as off-topic.
youknowriad
force-pushed
the
update/zoom-out-toolbar
branch
from
196ecc1
to
3e1e1fe
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
I think we should get some more feedback from design folks on this before we proceed. There are a number of PRs in the works that improve the vertical toolbar so it's important we don't discard it prematurely. @draganescu also raised some valid UX concerns about dropping it. Pinging @WordPress/gutenberg-design for feedback - specifically @richtabor (originally proposed the UX) and @jasmussen (Design Lead for 6.7). |
youknowriad
force-pushed
the
update/zoom-out-toolbar
branch
from
3e1e1fe
to
1797436
Compare
|
Thanks for the efforts all around, both to improve the vertical toolbar, and to remove it if need be. Ultimately what we want is for the zoom out editing experience to be a good one, that feels polished, stable, and holding promise for future iterations. With limited time before the RCs, what we have before is essentially a few options:
I would support either of those paths forward, and would be happy to defer to developer input on why one path may be better to take than the other. |
It seems like this could be related to #66138, though I'm not sure if you meant a reload by "going in and out". I don’t have a comprehensive enough perspective to opine on whether to keep the vertical toolbar. I will say it seems like a challenge if we ever want to support the "Show button text labels" preference. |
ellatrix
approved these changes
Oct 16, 2024
|
A few notes I've noted:
CleanShot.2024-10-16.at.09.19.38.mp4
|
I think this can be part of #66105, which I started working on. I'd also think we should remove it and have more consistency. |
|
@richtabor I addressed all the points. |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
Flaky tests detected in 3b037ed. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11382303274
|
|
There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch. PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk. |
|
Taking care of the cherry-picks. |
youknowriad
added a commit
that referenced
this pull request
Oct 17, 2024
Co-authored-by: youknowriad <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: jameskoster <[email protected]>
|
The cherry-pick is ready here #66200 I had to include some other changes from another PR but I think it's ready, I'd appreciate some testing. |
ntsekouras
reviewed
Oct 17, 2024
| size="compact" | ||
| /> | ||
| ) } | ||
| { canMove && canRemove && ( |
There was a problem hiding this comment.
Should we add these checks where we render this button? I think it was missed, unless it was intentional..
There was a problem hiding this comment.
Do you have concrete examples of when this causes any issue? For instance the "canMove" is clearly unnecessary to me but I'm wondering if canRemove is needed.
There was a problem hiding this comment.
canMove I agree with that and don't have the context for that. canRemove I think it's valid since we essentially removing it by replacing with a different pattern.
|
This was merged before I got to it. I'm happy with the decision if the Design Lead (@jasmussen) and the key folk working on the feature feel it's the right direction for stability and consistency in 6.7. @ndiego @colorful-tones Just an FYI about this change which is effectively reverting to improve stability of the Zoom Out feature for 6.7. I think we should also post an update on the Zoom Out tracking Issue to make folks aware of this change. |
|
Zoom is a great feature, and I there's all sorts of good reasons for a vertical toolbar. Equally there are good reasons for keeping a single recognizable block toolbar. Because of the latter, and to free up dev cycles, it makes sense to me to revisit this in the 6.8 cycle. |
jeryj
reviewed
Oct 17, 2024
Comment on lines
+67
to
+72
| return ( | ||
| <BlockPopoverInbetween | ||
| previousClientId={ previousClientId } | ||
| nextClientId={ nextClientId } | ||
| > | ||
| <ZoomOutModeInserterButton |
There was a problem hiding this comment.
We lost the previous inserter. This only shows the inserter after the selected block. There should be one before and after the selected block.
There was a problem hiding this comment.
This was a request by @richtabor
There was a problem hiding this comment.
Yes, it interfered with the block toolbar.
There was a problem hiding this comment.
It will still interfere with the toolbar when you scroll down though, right? Is there a solution we can have without removing it entirely? The inserters don't render until they're present on the screen, so removing the top inserter highlights an issue where there is no focusable Add pattern button available for the keyboard on patterns that extend past the bottom of the viewport.
There was a problem hiding this comment.
The inserters don't render until they're present on the screen, so removing the top inserter highlights an issue where there is no focusable Add pattern button available for the keyboard on patterns that extend past the bottom of the viewport.
@jeryj Would you be able to raise an Issue for that one? Any suggestions on a fix would be good. I assume it's "add back the inserters" but that will require input from Design so feel free to ping the design group here.
youknowriad
added a commit
that referenced
this pull request
Oct 18, 2024
Co-authored-by: youknowriad <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: jameskoster <[email protected]>
youknowriad
added a commit
that referenced
this pull request
Oct 18, 2024
Co-authored-by: youknowriad <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: ajlende <[email protected]> Co-authored-by: talldan <[email protected]>
youknowriad
added
Backported to WP Core
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
Co-authored-by: youknowriad <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: jameskoster <[email protected]>
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.
Related #66018 (comment)
closes #65985
closes #65570
What?
I know we've spent some time trying to make the zoom-out vertical toolbar but so far it has proven difficult to get it right and consistent without too many downsides. Given that the added value is not entirely clear, I'm proposing to just remove the toolbar and consistently use the regular toolbar.
Testing Instructions
1- Trigger zoom-out.
2- The normal toolbar is used for all sections.