Select Mode: Use the content-only behavior in select mode #65204
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
the
[Type] Technical Prototype
|
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. |
youknowriad
commented
Sep 10, 2024
| @@ -40,83 +40,38 @@ function BlockStylesPanel( { clientId } ) { | |||
| ); | |||
| } | |||
|
|
|||
| function BlockInspectorLockedBlocks( { topLevelLockedBlock } ) { | |||
There was a problem hiding this comment.
Instead of creating a custom "inspector control" for "content only" mode (and similar), I refactored this component to reuse the same block inspector control component by passing a "isContentLockingParent" flag instead.
There was a problem hiding this comment.
This makes everything easier to reason about 👍
youknowriad
commented
Sep 10, 2024
| </PanelBody> | ||
| ) } | ||
|
|
||
| { ! isContentLockingParent && ( |
There was a problem hiding this comment.
Right now, we hide all "inspector controls" because our APIs Slot/Fill assume that we always want to render the "selected block"'s controls. It's not the case for "content only".
There was a problem hiding this comment.
I think that's ok for now in this PR. We'll want to follow up shortly afterwards to see if we can render the controls for the selected block.
Alternatively we can explore drill down.
youknowriad
commented
Sep 10, 2024
youknowriad
commented
Sep 10, 2024
youknowriad
commented
Sep 10, 2024
youknowriad
commented
Sep 10, 2024
| templateLock === 'contentOnly' || | ||
| ( editorMode === 'navigation' && | ||
| ! sectionsClientIds.includes( clientId ) ) | ||
| ) { |
There was a problem hiding this comment.
The change here is what "flattens" the tree in "navigation" mode.
|
Size Change: -2.11 kB (-0.12%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Sep 10, 2024
youknowriad
requested review from
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
The change in select mode means the "block moving mode" doesn't work anymore. I decided to remove it in the last commit for the following-reasons:
|
getdave
reviewed
Sep 11, 2024
There was a problem hiding this comment.
Thanks for bringing together the ideas in a coherent PR 👍
What I like about this PR:
- the canvas selection is flattened - I can only select "content"
- the block toolbar is simplified - although we may need to tweak what is/isn't shown.
Things to consider for improvement:
- we should hide the
<BlockVariationTransforms>component. On the Group block especially it's a "layout" thing and should be avoided. - in List View removing any
blockEditingMode === disabled"container" type blocks (e.g. Group, Columns, Cover...etc) that are descendants of the "sections" (i.e. are not the sections themselves) would help to simplify the tree for users. A lot of the complexity comes from too many layout blocks being visible. - we could consider using a "Dark" block toolbar - I think this will help signify to the user that they in a particular mode.
- (probably one for a follow up) on many blocks the available controls are too limited. For example on the Heading block we should allow changing the level.
- a couple of times I saw the List View completely flatten itself. This seemed to be an intermittent bug and I wasn't able to narrow down the reproduction steps. I will update here when I figure it out.
- I couldn't hit
ENTERto add a new paragraph.
|
I think we need to codify what this means. What controls do we want to allow an why. So far this PR is about "content only" mode. But "level" is not considered content. What is the impact of this on "patterns" or "contentOnly" locking... We shouldn't be thinking about this mode in isolation. Right now this "content lock container" mode is enabled for these three cases:
When a container is considered "content lock container", it triggers a few things:
This is good for me. But it assumes that we want to keep a consistent behavior between these cases. Do we really want that? If not, why? and what are these differences. For instance, when you say "allow level" for Heading. Sure, I can see how that is useful for "select mode" but do we want the same for "patterns/reusable blocks"? Also, how to codify that: is it a block author's decision? is it something based on attribute roles (level is not content)... |
I agree and I appreciate the prompt to look at this holistically. Thanks for raising the other use cases. I'm aware of (most) of them and I haven't forgotten about them. In this review however, I was specifically focusing on the UX experience of how editing in a "simplified mode" feels. What are the benefits/drawbacks...etc. This is because you specifically referenced prior work in this area as the genesis of this PR. My thought was once we know how we want this experience to feel, we can cross reference with other potential use cases (patterns...etc) and decide if there is sufficient overlap or whether we need a different approach between the various scenarios. For example, some things we might wish to enable in a "simplified mode" for general editing would not work well with Patterns. Happy to dive into the various scenarios and codify the differences in a more indepth review. |
youknowriad
force-pushed
the
update/content-only-select-mode
branch
from
66a01ed
to
df55f3a
Compare
|
@getdave I realize that I didn't clarify my opinion on your previous behavior suggestions. Sometimes I have the tendency to share a minimal reply. I think the suggestions are great. I would add that maybe we should also show the "block toolbar" for the "content locking container" in addition to the toolbar of the selected block. And maybe remove the "block transforms..." from the selected block (treat it like an inline toolbar) |
I do miss being able to select the content locking container (pattern). |
|
To avoid bike shedding on the name of the modes here, I also added an item to to the issue's todo list. |
talldan
reviewed
Sep 19, 2024
Comment on lines
-297
to
-299
| canMove, | ||
| onMoveTo, | ||
| onlyBlock, |
There was a problem hiding this comment.
They might be unused internally, but I'm wondering whether these would be considered a public API if the slot fill is public.
There was a problem hiding this comment.
You're right. But IMO, these are bad props to start with and that are very specific to the "move to" feature that was removed.
| templateLock === 'contentOnly' || | ||
| editorMode === 'navigation' | ||
| ) { | ||
| // Sections should always be contentOnly in navigation mode. |
There was a problem hiding this comment.
I'm also wondering if the sectionClientIds logic should be mixed with the templateLock logic. Could there ever be a situation where the section root has templateLock: 'contentOnly' and then the sections logic unexpectedly applies? Not sure, maybe it's nothing to worry about 😄
talldan
approved these changes
Sep 19, 2024
mtias
approved these changes
Sep 19, 2024
|
I think this PR caused a negative impact on the "typing" performance metrics. Around 5% to 10%. This is most likely due to the increased complexity of the |
|
In the post editor, it seems that unintended button text and help text are being displayed in the toolbar dropdown. See #65548. |
|
Ok I know what the regression is and I know how to fix but in my testing, there's no single place where the "Edit template" (and its help text) should be shown because if you're in "template-locked" mode, you can't actually select blocks that are in the template, so I'm thinking of just removing that code. |
|
@youknowriad, was #65560 the fix for the performance regression you mentioned? |
|
@Mamaduka No, I'm not sure yet exactly what's the code that is triggering the small performance regression. |
22 tasks
|
This PR missed to update the gutenberg/packages/block-editor/src/store/actions.js Lines 1713 to 1724 in 75c2147 |
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.
Alternative to #65027
Unifies the approach in #65027 with the one in #65055
This PR tries to implement the behavior that was explored in #65027 but instead of introducing a new rendering mode, this replaces the "select tool"'s behavior with it.
Notes
Testing instructions