Only check for 'other' block control slot in useHasAnyBlockControls #59884
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: +53 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
|
@youknowriad Focus average is down 7-8ms for this PR over trunk. This can be a temporary solution that has a real improvement, but I think we need a long-term design solution for block pattern override/contentOnly toolbars. |
| const hasBlockToolbar = useHasBlockToolbar(); | ||
| if ( ! hasBlockToolbar ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Moving this check to before the <PrivateBlockToolbar /> avoids making an unnecessary useSelect, as if this is false then we know the toolbar will not render.
| * | ||
| * TODO: Remove this hook, as having a toolbar with only a Replace button is a | ||
| * misuse of the toolbar. | ||
| */ | ||
| export function useHasAnyBlockControls() { |
There was a problem hiding this comment.
The name of the hook, if the change remains, should be updated to something like useHasOtherBlockControls.
There was a problem hiding this comment.
I considered it too. You're right, the name should get updated if we go with this change.
It's still possible to add items to all the So, I believe everything is still accounted for by only checking the other slot, since we only care about this in contentOnly mode. It's already a hack of the toolbar, IMO, so I don't think we need to account for third party usage of this. |
|
@jeryj, Sure, the core might be using the P.S. Do you mind rebasing on top of the latest trunk? The Quote block changes landed last week, and it would be interesting to test this proposal using them. |
|
I share @Mamaduka 's concern in my comment, that we can't assume that no controls in |
|
I'll rebase and see. Maybe I was looking at the old block code.
My point is that it shouldn't have been allowed to be done this way in the first place, so let's prevent future contentOnly mode blocks from using other slots to contain the issue while we come up with a better solution. It's a real performance issue to allow it to continue checking for all slots. This is a way to let it keep working as is and bring a peformance improvement to the toolbar. |
The useHasAnyBlockControls hook is a hefty way of checking if we are in contentOnly mode for image and featured image blocks. This reduces the performance impact while we figure out what to do in a more permanent way.
jeryj
force-pushed
the
perf/use-has-any-block-controls
branch
from
8586400
to
6f3db75
Compare
|
@Mamaduka - I've rebased. Could you direct me to the changes to the Quote block you are concerned about related to this PR? I've tested it out and don't see anything different between this PR and trunk, and these are the only |
The Here's the example block for testing. <!-- wp:group {"templateLock":"contentOnly","layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:quote -->
<blockquote class="wp-block-quote"><!-- wp:paragraph -->
<p>Hey there!</p>
<!-- /wp:paragraph --></blockquote>
<!-- /wp:quote --></div>
<!-- /wp:group -->The block toolbar is visible on the trunk with the Quote block's content controls.
The toolbar is no longer visible on this branch.
|
|
Thanks for helping me understand this, @Mamaduka! Sorry if I'm coming across as combative. I really do appreciate your help and want to figure this out!
Even though
My argument is that you can use the block toolbar in contentOnly mode, but you're not supposed to be able to. The quote block having a block toolbar when in Update: Playing around with it more. Is the behavior supposed to be a reduced number of tools when in contentOnly mode? Which groups are allowed or not in contentOnly mode? If all slots are supposed to be allowed, is there a more performant way we can check for it? |
|
This has been really helpful for me to better understand the intention of contentOnly. So, any tools related to editing content are allowed, but not layout. And the block toolbar doesn't know which tools should be allowed in contentOnly mode or not, so any slots need to be available for rendering. Does this sound right? If that's the case, then would a way to indicate a tool is a content tool vs layout tool be useful? Are there any other ways we can determine this without checking all the slots? |
|
@jeryj, no worries at all. I appreciate our discussion here.
That sounds correct. Valtio powers the SlotFills store; I wonder if we can compute a derived state that indicates the existence of Fills - |
What breaks? Can we solve that in a different way without checking for the presence of fills in any slots?
|
I'm going to try removing the slot fills check entirely and see if we can fix its issues a different way. |
|
Closing in favor of #60717 |
The useHasAnyBlockControls hook is a hefty way of checking if we are in
contentOnlymode for the image and featured image blocks. This reduces the performance impact from #58979 while we figure out what to do in a more permanent way.Other ideas:
1. Have a non-block toolbar popover for content-only items in image/featured image: It only uses a Replace button, and it's quite weird when Top Toolbar mode is on. For example, the Replace button is in the top toolbar here without any image block icon or other toolbar items. What am I replacing? I think this should only be a floating button in a popover, not something that relies on the block toolbar. There are now more tools being added to Pattern overrides, so maybe this should be its own section within the block toolbar, or a separate block toolbar?
2. Write specific overrides into the block toolbar to account for contentOnly toolbar items Is this a misuse of the toolbar? It seems intended to not render in contentOnly mode (as no other toolbar items show), so if we do want to use it, maybe we should specifically write in these instances rather than allow for an empty-looking toolbar?
What?
Improve the performance of Selecting blocks.
Why?
Performance improvement.
How?
The
<MediaReplaceFlow />used by the image and featured image are in the "other" slot. Let's not check for all slots when we can do the check once.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast