-
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: light block edit/save symmetry #25644
Conversation
Size Change: +190 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
58d34c6
to
568d0be
Compare
568d0be
to
3f6715a
Compare
@@ -2,13 +2,14 @@ | |||
* WordPress dependencies | |||
*/ | |||
import { RichText } from '@wordpress/block-editor'; | |||
import { getBlockProps } from '@wordpress/blocks'; |
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 actually wonder about the best place to export this, maybe useBlockProps.save()
?
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.
That's possible too. I don't mind either way. I'll update 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.
This looks much nicer actually! :)
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'm not sure how I feel about exposing it through useBlockProps.save()
. It feels a little weird to me, but I also don't outright dislike it. I'm okay with it either way. I suppose one benefit is that it more closely ties it to useBlockProps
.
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 reasoning for proposing this is that later we might have useInnerBlocks.save()
, useRichText.save()
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, I like this pattern. Good idea!
I like this personally, i'd like more opinions here @mcsf @mtias @jorgefilipecosta |
b32086b
to
84aeb9e
Compare
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
100e873
to
062d519
Compare
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.
Having more opinions is better but this is looking good for me.
I'll let it sit over the weekend. Would be nice to ship early Monday so we can stabilise it for the release. |
Nice work here. There are some aspects that I like:
What I don't like as much:
All in all, I lean more towards yes than no, but it feels a little like a step in the dark. |
It doesn't really matter, you can do both. You can also pass the props to
Similarly, |
Nothing would break, since this API is opt-in. |
Let's do it. :) |
Description
Replaces the earlier attempt #20721.
Things have changed now with the replacement of the block component by a hook in #23034.
This must be done before stabilising the new (light) block API, because we're using the supports flag to adjust serialisation.
It creates a bit of consistency in the API, the only difference being that for
edit
it's a hook adding behaviours and forsave
it's a simple functionuseBlockProps.save
to insert (filtered) attributes on the block wrapper.How has this been tested?
Screenshots
Types of changes
Checklist: