Select Mode: Updates to the block toolbar #65485
Conversation
youknowriad
added
[Type] Enhancement
| if ( isEmpty ) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the only change to this file, the isEmpty flag was added and the dropdown is not rendered if empty.
|
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. |
| @@ -35,36 +35,40 @@ function BlockSwitcherDropdownMenuContents( { | |||
| clientIds, | |||
| hasBlockStyles, | |||
| canRemove, | |||
| isUsingBindings, | |||
There was a problem hiding this comment.
No need to pass this prop from up to down, it's better to collocate the call.
| @@ -196,7 +200,7 @@ const BlockIndicator = ( { icon, showTitle, blockTitle } ) => ( | |||
| </> | |||
| ); | |||
|
|
|||
| export const BlockSwitcher = ( { clientIds, disabled, isUsingBindings } ) => { | |||
There was a problem hiding this comment.
Collocated both of these props.
| ( isDefaultEditingMode || | ||
| ( isContentOnlyEditingMode && ! hasParentPattern ) || | ||
| isSynced ) && ( | ||
| ! hasParentPattern && ( |
|
Size Change: +58 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Love this! Feeling much more like an editing experience centred around page building using patterns. |
|
Yeah this feels like a step forward 💪 Some observations:
inserter.mp4 |
youknowriad
requested review from
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
youknowriad
force-pushed
the
update/block-toolbar-edit-mode
branch
from
973b495
to
fad0d9e
Compare
|
Flaky tests detected in b941814. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10958244637
|
I'm most likely not understanding this point, for me there's no inconsistency, can you clarify? |
Both of these are things we can explore. I want to focus on the minimal changes to the toolbar for the current PR (just reuse what we have already).
I will look into it. It might be a bit hard to know whether there's a tool or not though 😬 (given all the extensibility APIs)
Yep, I can reproduce, I'll look into it but that feels separate from the current PR though, we should add it to the todo list in the issue to track it/fix it. |
Yes, so it's actually very hard to "detect" when a toolbar is empty or not. My best guess at the moment would be to actually do some CSS hack with Given that we want to explore adding names and potentially shuffle buttons, I was wondering whether the current state could be an acceptable tradeoff for this PR. |
youknowriad
force-pushed
the
update/block-toolbar-edit-mode
branch
from
fad0d9e
to
b941814
Compare
|
@jameskoster The bug about the in between inserter showing up is being fixed separately here #65529 |
Sure, see the video below: template.mp4
Edit: I noticed the section does appear in list view if you move it: group.mp4
Probably okay, though for this PR it would be good to fix the appearance of a 'double border' on the right-hand side of the toolbar: 
One other question; is it intentional that I cannot select the title or featured image when editing a page in edit mode? |
I can't reproduce the issue, for me it works. I'm suspecting there's a bug in the way some of the selectors are memoized. Causing different results depending on what you did before.
Well, these are not "content" blocks, so yeah, it is just the algorithm of "contentOnly" basically. |
|
The double border issue is fixed. Note that if you notice an editor crash (some kind of infinite loop) when testing this PR, it's mostly likely due to this unrelated PR #65494 (ignore that error for now) |
I suppose it should be discussed separately but I feel like there may be a nuance there. I suspect folks would expect the title to be editable regardless of mode. |
|
Ok, I found the bug where sometimes the list of blocks in list view is not refreshed properly. It exists in trunk too but I included the fix here in the last commit (it's a small one) ac423e4 |
youknowriad
force-pushed
the
update/block-toolbar-edit-mode
branch
from
ac423e4
to
b5476d9
Compare
| <BlockSettingsMenuControls.Slot | ||
| fillProps={ { | ||
| onClose, | ||
| count, | ||
| firstBlockClientId, | ||
| } } | ||
| clientIds={ clientIds } | ||
| /> |
There was a problem hiding this comment.
It looks like in some cases this slot will output the menu group, even when it's visually empty. To reproduce, if I use Edit mode and select a list item within TT4's blog home template, the settings menu will only include Duplicate as an option, but there'll be this extra empty area:
With a bit of commenting out, I think I've identified the three components that are rendering empty fills. They bail before outputting anything visually, however it results in BlockSettingsMenuControlsSlot thinking that there are three fills here and so the MenuGroup gets rendered which creates the extra space. Here's what I've commented out:
diff --git a/packages/editor/src/components/provider/index.js b/packages/editor/src/components/provider/index.js
index 11b1478d584..627e32e588f 100644
--- a/packages/editor/src/components/provider/index.js
+++ b/packages/editor/src/components/provider/index.js
@@ -327,17 +327,17 @@ export const ExperimentalEditorProvider = withRegistryProvider(
selection={ selection }
settings={ blockEditorSettings }
useSubRegistry={ false }
>
{ children }
{ ! settings.__unstableIsPreviewMode && (
<>
- <PatternsMenuItems />
- <TemplatePartMenuItems />
- <ContentOnlySettingsMenu />
+ {/* <PatternsMenuItems /> */}
+ {/* <TemplatePartMenuItems /> */}
+ {/* <ContentOnlySettingsMenu /> */}
{ mode === 'template-locked' && (
<DisableNonPageContentBlocks />
) }
{ type === 'wp_navigation' && (
<NavigationBlockEditingMode />
) }
<EditorKeyboardShortcuts />That then fixes up the spacing:
So, in order to fix that spacing, do we need to conditionally render the above components, or update their internals so they don't output <BlockSettingsMenuControls> unexpectedly?
Initially I was wondering if we could fix it by using CSS with something like:
.components-menu-group:has( > div:empty ) {
display: none;
}But unfortunately that still leaves a space due to the last menu group needing the negative margin in order for the spacing to look correct. And even with display: none, this empty group winds up getting that rule instead of the one that's visually there:
So, unless there's another CSS-only way to handle it, we might need to lightly refactor some of those components that are outputting empty fills?
There was a problem hiding this comment.
Unfortunately, it's going to be very hard to solve these things by refactoring the fills. This is not the first item or the only place where we have this issue. I wonder if it's something fundamental to how slot/fills work.
If we can't find a CSS-only way for now, I think we should accept a small inconvenience for now.
There was a problem hiding this comment.
Ok I think I have a solution here but also, don't want to apply it in this PR though because it's potentially impactful. So I'm going to open a separate PR for that.
There was a problem hiding this comment.
This PR should fix it #65561
There was a problem hiding this comment.
I'm seeing a bug.
In any block even a paragraph if we go to the block settings we see this message:
"Edit the template to move, delete, or make further changes to this block."
I think this message does not make sense, but checking on the trunk it also seems to appear so it is not a blocker.
Related to the above, I'm also seeing other changes: |
|
Yes, that bug is being fixed here #65560 |
Builds on top of #65204
This PR makes some changes to the block toolbar behavior for "sections" (content-only containers, select mode, reusable blocks...) and simplifies things a little bit.
Here's the list of changes that I made so far:
Testing instructions