One hook to rule them all: preparation for a block supports API #56862
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
ellatrix
changed the title
|
Size Change: -575 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in aa5e8f5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7132310759
|
ellatrix
commented
Dec 7, 2023
| ); | ||
| export default { | ||
| edit: BlockEditAlignmentToolbarControlsPure, | ||
| attributeKeys: [ 'align' ], |
There was a problem hiding this comment.
@youknowriad I'm using this to "subscribe" to specific attributes only. But on the other hand, extra useSelect subscription these control components are not bad per se because they are only rendered when a block is selected. If we allow that, we wouldn't need to do this declaration of which attributes need to be passed down. Either way, I'm fine with it, they're not mutually exclusive.
There was a problem hiding this comment.
And we can decide this later when we actually make the real API.
There was a problem hiding this comment.
Yes, this is totally fine for me and to be honest, I'm not entirely sold on opening this API to the public yet but it's definitely a great improvement for us.
There was a problem hiding this comment.
Oh sure, no public API for now :D
youknowriad
reviewed
Dec 7, 2023
| POSITION_SUPPORT_KEY | ||
| ); | ||
| export default { | ||
| edit: function Edit( props ) { | ||
| const isPositionDisabled = useIsPositionDisabled( props ); |
There was a problem hiding this comment.
This one is interesting, I feel like maybe we need one sync hasSupport key and potentially another useHasSupport key that can use hooks. (if this is more common)
There was a problem hiding this comment.
We can alway add that later if needed. So far it's only one of them.
youknowriad
reviewed
Dec 7, 2023
There was a problem hiding this comment.
This is really cool. I like where this is going. Let's obviously keep that kind of "API" private but it clearly shows that we're repeating the same things over and over and that we can optimize these.
Also I believe ultimately, that we should avoid using the filters entirely and just bundle these in the framework directly.
Yes, eventually, we should create a store and mount this component straight in the right place instead of with filters. |
youknowriad
approved these changes
Dec 7, 2023
|
@youknowriad Probably not, it's just easier to oversee everything and not make performance regressions. |
That is exactly my thinking about the perf implications since it allows getting rid of at least a single higher-order component per feature! I couldn't agree more! Excellent refactor, @ellatrix! We don't even have to commit to any public interface for now. It's going to be discovered at the end of applying all necessary changes. |
youknowriad
reviewed
Dec 7, 2023
| @@ -153,6 +153,7 @@ function BlockEditAlignmentToolbarControlsPure( { | |||
| } | |||
|
|
|||
| export default { | |||
| shareWithChildBlocks: true, | |||
There was a problem hiding this comment.
I think I'd have preferred to leave this one to its own PR, to have a clear picture of impact.
There was a problem hiding this comment.
It's needed, otherwise the controls are not rendered. For example for a button block, the align controls from the buttons blocks should be rendered.
ellatrix
force-pushed
the
try/prep-block-support-api
branch
from
3c2eaf2
to
aa5e8f5
Compare
|
This is awesome, thank you! |
ockham
reviewed
Dec 15, 2023
There was a problem hiding this comment.
👋 I might be missing some context, so I'm not sure it matters, but I just came to say that Block Hooks are not using the Block Supports mechanism; they're separate.
There was a problem hiding this comment.
It doesn't matter, we can just turn on support for all blocks. The point is that controls are managed in one place and only rendered and re-rendered when necessary. In this case, since the controls don't depend on any attributes, we can prevent unnecessary re-renders.
ockham
reviewed
Jan 18, 2024
| break; | ||
| } | ||
|
|
||
| const hookedBlock = candidates?.find( | ||
| ( { name } ) => name === block.name | ||
| ( candidate ) => name === candidate.name |
There was a problem hiding this comment.
I just noticed that this broke the toggle 😬
I've filed a fix: #57956.
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.
What?
This PR refactors to hooks to use a single filter for
editor.BlockEdit, and with that lays the foundations for a new block supports API (the other filters remain to be done in a similar fashion).Why?
Related to #51005.
Less code duplication.
Less higher order components and less components in a block tree.
Less opportunities for performance regressions:
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast