Block editor: hooks: manage BlockListBlock filters in one place #56875
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
|
Size Change: -151 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 5279781. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7144463769
|
ellatrix
force-pushed
the
try/hook-use-block-props
branch
from
bf71f98
to
2191923
Compare
|
Last run: -15% on type. |
|
Second run -28%? 🤔 |
youknowriad
reviewed
Dec 8, 2023
| @@ -155,54 +154,27 @@ function BlockEditAlignmentToolbarControlsPure( { | |||
| export default { | |||
| shareWithChildBlocks: true, | |||
| edit: BlockEditAlignmentToolbarControlsPure, | |||
| useBlockProps, | |||
There was a problem hiding this comment.
Maybe useBlockEditProps, I can see us having useBlockSaveProps, or useBlockProps for both?
There was a problem hiding this comment.
Maybe we should keep it consistent with the block API? useBlockProps and useBlockProps.save? One is a hook and the other one is static.
There was a problem hiding this comment.
But yes, it's not entirely the same so maybe a different naming is appropriate.
youknowriad
reviewed
Dec 8, 2023
| <BlockListBlockWrapper | ||
| Component={ BlockListBlock } | ||
| useBlockProps={ useBlockProps } | ||
| attributeKeys={ attributeKeys } |
There was a problem hiding this comment.
I think there's a subtle issue here. useBlockProps is a prop so theoretically it can change over time (I know it can't but we should support it can). And it's also hook so to avoid breaking the rules of hook, we should "remount" the BlockListBlockWrapper component each time useBlockProps changes. So I think we should handle this by generating a unique key prop and change it, each time useBlockProps change (I do something similar in the commands palette to support command loaders)
There was a problem hiding this comment.
We create a unique component for each hook? So we're not breaking any rules?
There was a problem hiding this comment.
Forcing "remounting" the component on each hook change should suffice, no need to create another component.
There was a problem hiding this comment.
Hm, I'll look into that.
There was a problem hiding this comment.
But it should also be fine as is, but sure, worth trying to remove the nested components.
There was a problem hiding this comment.
It's fine yes, but there's an unwritten assumption that useBlocksProps is a static prop that never changes. I'm fine with a comment if you prefer. But it means we should never allow it to be dynamically changed (like using a filter or anything)
youknowriad
approved these changes
Dec 8, 2023
ellatrix
force-pushed
the
try/hook-use-block-props
branch
from
daab325
to
a40e20d
Compare
ellatrix
force-pushed
the
try/hook-use-block-props
branch
from
a40e20d
to
2526095
Compare
ellatrix
force-pushed
the
try/hook-use-block-props
branch
from
2526095
to
5279781
Compare
|
18% in the last run. Let's merge and iterate on this. |
jsnajdr
reviewed
May 23, 2024
| const shouldRender = selector && attribute; | ||
|
|
||
| useDuotoneStyles( { | ||
| clientId: id, |
There was a problem hiding this comment.
@ellatrix I noticed this when refactoring block refs in #60945: you're passing an instance ID instead of the block's clientId, and the useBlockElement( clientId ) will never get the block element, and the Safari bugfix won't work.
This is a bug introduced by this PR. The original implementation in #55288 by @andrewserong was correct.
I don't know how to fix it: the useBlockProps hook doesn't receive clientId or any other block information as a prop.
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?
Best checked without whitespace
Similar to #56862, but for the BlockListBlock filters.
Related to #51005.
I've shaped the internal API around "block props". These filters never add more than wrapper props to the block, or in the case of duotone and layout, add editor styles and a class name.
To do: add the rest of them :)
Why?
Reduces code duplication.
Allows us to manage the filter and do performance optimisations in one place.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast