Closed
Conversation
|
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: -435 B (-0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
getdave
reviewed
Sep 24, 2024
Contributor
getdave
left a comment
There was a problem hiding this comment.
Nice exploration. A few issues come to mind:
Broken Drag and Drop
This breaks drag and drop for me. Or at least it reverts the previous behaviour which "displaced" the sections to show the zoom out drop target when dragging over.
Can we resolve that?
Keyboard accessibility
With the removal of the + inserters how would a keyboard user access the Patterns tab search?
I appreciate that currently in trunk this interaction is also broken for keyboard users, but the intention was to focus the patterns search if/when the inserter was clicked.
Contributor
Author
|
Good catch @getdave I've restored the expand on hover behaviour but it's a bit different now. |
ff0cae0 to
fa43644
Compare
Contributor
|
Thanks for update. I wonder if when you drag over the block it should open both drop zones? At the moment they open/close depending on where you are dragging and it doesn’t feel consistent.
|
Contributor
Author
|
It's not that bad. It's fine that both open, given they're indicators to drop before or after. What annoys me is that only one of them stays open if the hover goes away from that edge. Any ideas? |
youknowriad
reviewed
Sep 25, 2024
youknowriad
reviewed
Sep 25, 2024
| ref={ blockToolbarAfterRef } | ||
| /> | ||
| { isZoomOutMode && ( | ||
| <ZoomOutModeInserters |
Contributor
There was a problem hiding this comment.
So this PR removes the always visible in-between inserters for zoom-out. Right? I think that's ok but should we restore "in-between" inserter in that case?
Contributor
There was a problem hiding this comment.
I guess it's a matter of removing the isZoomOut here
Contributor
Author
There was a problem hiding this comment.
Let me try this and see what happens.
youknowriad
reviewed
Sep 25, 2024
| setZoomLevel( 50 ); | ||
| setIsInserterOpened( { | ||
| tab: 'patterns', | ||
| category: 'all', |
Contributor
There was a problem hiding this comment.
In my test, if I "toggle zoom-out" mode, the inserter opens with the "all" category selected.
But If stay in zoom-out and close and then reopen the inserter, I'm not sure I understand why it shouldn't have the same effect (open "all" category).
The other question is where is the code responsible of enforcing "patterns" tab by default if I toggle the inserter in zoom-out? I think that code should do the same for "navigation" mode because otherwise, if we switch the "mode" when zoom-out is enabled, we kind of fallback to "blocks" by default.
Contributor
Author
There was a problem hiding this comment.
But If stay in zoom-out and close and then reopen the inserter, I'm not sure I understand why it shouldn't have the same effect (open "all" category).
I think I subscribe to the intention behind the above, I am unsure if having the inserter button doing different things depending on the view is OK or confusing.
Contributor
There was a problem hiding this comment.
I think I subscribe to the intention behind the above, I am unsure if having the inserter button doing different things depending on the view is OK or confusing.
I'm also not sure 100% but I think maybe we should give it a shot separately and see.
What about my second question, do you have any idea?
youknowriad
reviewed
Sep 25, 2024
Contributor
youknowriad
left a comment
There was a problem hiding this comment.
Assuming folks @WordPress/gutenberg-design agree with the removals of the always visible inserters, this PR is good to me.
I left some comments.
Contributor
Personally, I don't like this space at all (neither top or bottom) because it doesn't really work well in all situations (especially if you have padding around the "main" section). I'm not entirely sure if it brings value. I think it does bring value when dragging blocks around but it can be enabled for all modes in that flow (drag and drop). |
Contributor
Author
|
I think this space can also serve well in an UX where the selected section also has content only editing enabled - this way is signifies a sort of :active state of the selected section where tiny edits can also be made (change image, update a heading). |
Member
|
A few notes:
CleanShot.2024-09-25.at.08.58.47.mp4
CleanShot.2024-09-25.at.09.00.00.mp4CleanShot.2024-09-25.at.09.01.20.mp4 |
Contributor
Author
Why? My reasons for suggesting this is to convey what the UI should simplify. What is are your reasons that make you be uncertain.
It is adding the notion of active or selected section, by separating it both top and bottom. The discontinuity is meant to make it more obvious and I think coupled with my idea above to enable content only editing for the selected section only it makes a good fluid experience. The second space that appears on hover bottom is a bug. |
Contributor
Author
|
I will close this and explore remaining issues separately. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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?
Simplify the UX of zoom out inserting:
Why?
The [+] buttons are obtrusive and inconvenient as an affordance. The separator that they create is an orphan in the UI - there is no relation between the inserter and the space created. Even with the prompt in #65392 it's still an awkward interaction that one clicks a button to get some text.
Opening the inserter in the pattern library as soon as entering zoom out is also more in tune with the original intent of this view to be useful for page assembly.
How?
Refactor some of the zoom out implementation. Remove the special inserters.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
alternative-zoom-out.mp4
To review the intereaction. Is it worth pursuing this path?