-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block API: deprecate experimental Block.* component #25515
Conversation
Size Change: -1 B Total Size: 1.17 MB
ℹ️ View Unchanged
|
The test failures (or at least one of them) are legitimate. I tried going through the steps of "Navigating the block hierarchy › should navigate using the block hierarchy dropdown menu" manually, and when I tried to perform the "tab twice" action on line 69, the inserter closed. No clue what's causing that issue yet, but I'm pretty sure it's not related to the tweak in |
@ZebulanStanphill Yes, it's a strange issue... it seems related to inner blocks and perhaps unnecessary re-renders. |
|
hasInnerBlocks={ innerBlocks.length > 0 } | ||
/> | ||
</TagName> | ||
<Content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's just me but I don't see what these two components bring aside indirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that you cannot use useBlockWrapperProps
without having an element that set a ref that you can add event listeners to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's the "return null" that causes the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. If you have multiple paths that are all creating a block, it would be fine to "share" the hook. Not sure if in this care we can just return a placeholder block or something. A block not returning anything is strange. There's a block, but you can't see it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's probably some hidden bug somewhere there but can be handled separately tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youknowriad I reverted the previous change and now it will always render a block. If the post ID is not resolved it will output "Loading..." Not sure if that's the best, but we can change the loading thing later on if needed. Imo we should always render a block even when it's still loading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, good fix, thanks for the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, some tests are failing now. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test everything here but the changes look good.
c4dcbc6
to
ff0b08f
Compare
98e18a2
to
6ee2199
Compare
Changes to cc @ellatrix |
Description
With the newly introduced
useBlockWrapperProps
in #23034, the experimentalBlock.*
component can be deprecated. Since plugins are likely to use this component (even though it's experimental), we should not remove it for at least a few releases.How has this been tested?
Screenshots
Types of changes
Checklist: