Block editor: new editor.BlockControls filter
#48809
Conversation
|
Size Change: +521 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
ellatrix
changed the title
editor.BlockControls filter
| className={ classnames( | ||
| props.className, | ||
| isEditingAsBlocks && | ||
| 'is-content-locked-editing-as-blocks' |
There was a problem hiding this comment.
This class is not used anywhere at all.
| @@ -20,6 +24,76 @@ import { BlockEditContextProvider, useBlockEditContext } from './context'; | |||
| */ | |||
| export { useBlockEditContext }; | |||
|
|
|||
| function BlockControlFilters( props ) { | |||
There was a problem hiding this comment.
Isn't this the same thing as just adding an empty component and wrapping it with withFilters or something like that?
There was a problem hiding this comment.
I guess there are a few differences:
- The filter doesn't have to render the previous value. This is a great improvement DevX wise.
- It adds a check
shouldDisplayControls: I'm guessing that's to improve performance and avoid rendering these controls if not necessary. While this is a good thing probably, it does add a bit of overhead in the sense that most of these filters actually renderInspectorControlsorBlockControlsthat do these checks as well. So using a context value for these checks might still be a good idea. Also, should we support the case where we pass flag to enable__experimentalExposeControlsToChildrenbehavior or not? (so let's say that this is generally a good thing) - The other thing is that folks can render anything in this filter and not just controls, like random Divs... So maybe, we should forbid that by adding some hidden wrapper or something like that or finding a way to check that it renders no DOM nodes.
There was a problem hiding this comment.
For the performance aspect, we should be a bit careful here to not actually degrade performance. Some existing hooks might have existed earlier (like by checking a block support) while here we first check shouldDisplayControls then check the block supports ... within the hook. So I wonder if we should first land shouldDisplayControls as a context value before this PR.
There was a problem hiding this comment.
While this is a good thing probably, it does add a bit of overhead in the sense that most of these filters actually render InspectorControls or BlockControls that do these checks as well. So using a context value for these checks might still be a good idea.
The overhead is almost negligible. I'd prefer keeping all this internal instead of exposing it through context, although this context in particular can be entirely internal if we don't use the existing context.
Also, should we support the case where we pass flag to enable __experimentalExposeControlsToChildren behavior or not? (so let's say that this is generally a good thing)
It supports it. The filters will render.
For the performance aspect, we should be a bit careful here to not actually degrade performance. Some existing hooks might have existed earlier (like by checking a block support) while here we first check shouldDisplayControls then check the block supports ... within the hook. So I wonder if we should first land shouldDisplayControls as a context value before this PR.
Not sure how this would degrade performance. We still guard hasSelectedInnerBlock with a __experimentalExposeControlsToChildren check. Using context for InspectorControls and BlockControls would be better yes.
There was a problem hiding this comment.
The filter doesn't have to render the previous value. This is a great improvement DevX wise.
That's correct. If this is a good pattern for other filters, at some point we could move this to the components package next to withFilters, but let's see how it goes.
There was a problem hiding this comment.
The other thing is that folks can render anything in this filter and not just controls, like random Divs... So maybe, we should forbid that by adding some hidden wrapper or something like that or finding a way to check that it renders no DOM nodes.
Yes, they could do this previously too. I do like that idea! We should be as restrictive as possible with this new filter.
There was a problem hiding this comment.
It supports it. The filters will render.
I mean some controls won't render if the block itself is not selected and that additional check and rendering is not necessary in this case.
There was a problem hiding this comment.
I mean some controls won't render if the block itself is not selected and that additional check and rendering is not necessary in this case.
I'm not sure I'm following 😅
There was a problem hiding this comment.
There's still some open questions but I generally like how this avoids the issues we might have with:
- Removing the need to ensure
BlockEditis rendered properly (unmounting issues...)$ - Take care of some initial performance optimizations. (though here we must be careful)
|
Flaky tests detected in 49e6aa6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4356473636
|
|
Ok, this is looking more and more as a great approach, I'd appreciate thoughts from @gziolo |
| withToolbarControls | ||
| ifCondition( ( { name } ) => hasBlockSupport( name, 'align' ) )( | ||
| ToolbarControls | ||
| ) |
There was a problem hiding this comment.
It's worth noting that these checks aren't contributing much to the performance improvement, but still, it's good practice to avoid as much calls to hooks as possible. Hopefully new hooks and plugins copy this pattern.
In general, you have my 100% support in the efforts to I was looking at PHP implementation for Block Supports, for example: gutenberg/lib/block-supports/spacing.php Lines 69 to 76 in e2e96cf Have you considered a more formal API that is based on the data store instead of further promoting filters? registerBlockSupport( 'core/anchor', {
isSupported( name ) {
return hasBlockSupport( name, 'anchor' );
},
updateAttributes( attributes )
if ( 'type' in ( attributes?.anchor ?? {} ) ) {
return attributes;
}
return {
...attributes,
anchor: ANCHOR_SCHEMA,
};
},
Edit( props ) {
return <EditAnchorControls { ...props } />;
}
} );Both I think it's close to what the community was trying to develop in the early days when struggling with the complexity of the filters-based approach. Edit: I quickly sketched the API that could get added in JavaScript to complement what exists in PHP. I would like you to treat it as a starting point in the discussion on what the developers could use. |
gziolo
added
[Feature] Block API
|
Yeah, I think ultimately, something like @gziolo's proposal would be awesome. Maybe it's an opportunity to start this as a private API (starting with "controls" key proposed in this PR) and enhance it slowly until we arrive to a state that we can confidently share. |
| return; | ||
| } | ||
|
|
||
| return blockControlFilters.handlers.map( |
There was a problem hiding this comment.
That's probably not quite how WordPress filters were designed to work. I'm not sure I've ever seen usage where the previous value would not get set at all. It works here really well though 😄
|
I like that proposal! Ok, let's lock the API here and then explore exposing it through |
| @@ -27,6 +27,8 @@ export const settings = { | |||
| }; | |||
|
|
|||
| export const init = () => { | |||
| // This is a temporary solution so the link is displayed at the top. | |||
| // See https://github.com/WordPress/gutenberg/pull/31833/files#r634291993. | |||
There was a problem hiding this comment.
@ntsekouras Any ideas on how to remove this temporary approach?
There was a problem hiding this comment.
I'm not sure since we don't want to expose blockControlsFilterName. Probably we could just lock it and then export it for use in Query Loop and adjust the filter in a similar fashion.
Also in a different direction this specific functionality could be part of a new SlotFill in block card/description. This is tracked in: #41236 as a task.
| @@ -24,6 +29,9 @@ import { store as blockEditorStore } from '../../store'; | |||
| */ | |||
| export { useBlockEditContext }; | |||
|
|
|||
| // Please do not export this at the package level until we have a stable API. | |||
| export const blockControlsFilterName = uuid(); | |||
There was a problem hiding this comment.
Yes, that might work as a first iteration 👍🏻
There was a problem hiding this comment.
Look like this approach creates flakiness in tests. Not sure why.
There was a problem hiding this comment.
Ah, figured out why. Hook names must start with a letter and uuid() will randomly also generate ids starting with a number.
ellatrix
force-pushed
the
try/dedicated-block-controls-filter
branch
from
f79ea8f
to
49e6aa6
Compare
|
For now, let's go ahead with this change. Here's an iteration on the interface @gziolo shared. If we base it on the support key, we don't even need registerBlockSupport( supportKey, {
// Can be merged with existing settings.
blockSettings: {
attributes: {
align: { type: 'string' }
},
},
Controls( { attributes, setAttributes } ) {
return <InspectorControls>;
},
// Maybe this can be optional and use getSaveProps by default.
useBlockProps( attributes, blockType ) {
return {
className: ...
}
},
getSaveProps( attributes, blockType ) {
return {
className: ...
}
}
} ); |
|
I any case this PR can be seen as preparatory work moving us in the right direction. |
Yes, it could work in most cases. I'm not sure about the case where the block supports entry has some complex structure with the nested objects, but maybe it isn't a concern. You should now have a better overview based on the refactoring applied so far.
Yes, it definitely could be landed separately with the current approach for a temporary filter that makes it impossible to use it for adding. The only question is whether sites or plugins use the existing filters to unregister controls when they don't want to show them in the UI. That might complicate the gradual rollout a little bit. Although, it isn't impossible to detect, so maybe we can account for that as well by checking whether current filters exist and bail out in the new implementation if they were removed with the assumption they were disabled. |
| @@ -395,7 +387,9 @@ addFilter( | |||
| withPositionStyles | |||
| ); | |||
| addFilter( | |||
| 'editor.BlockEdit', | |||
| 'editor.BlockControls', | |||
There was a problem hiding this comment.
I think this should be blockControlsFilterName.
What?
This PR is easier to review without whitespace changes.
All use cases for the
editor.BlockEditfunction are adding block controls. We should create a less powerful filters specifically for adding block controls that only renders these when needed.This is beneficial for performance, and it's easier to understand without the HoC.
For now this filter is private until we're ready to share an interface we want to expose.
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast