Block Library: Move supports to block.json files for core blocks
#22422
Conversation
gziolo
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta,
mkaz,
mkevins,
nerrad,
ntwb,
pinarol,
Soean and
talldan
as code owners
gziolo
added
[Package] Block library
|
Size Change: +45 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
gziolo
force-pushed
the
update/block-json-supports
branch
from
e271143 to
815cca2
Compare
I'm going to remove them and open an issue with a list to address it separately. It isn't clear to me why it can't be handled in the place that handles the |
gziolo
changed the title
supports to block.json files for core blocks
gziolo
force-pushed
the
update/block-json-supports
branch
from
815cca2 to
68c7157
Compare
Blocks that should be tackled separately because of the condition in With that, I can remove those comments :) |
gziolo
force-pushed
the
update/block-json-supports
branch
from
68c7157 to
5a39992
Compare
|
@Soean and @ZebulanStanphill – thanks for your feedback, I addressed both issues. |
|
There is one more thing to clarify since I mentioned that I could split this PR into smaller parts. Given that I had to rebase this PR already, I would be more than happy to land all changes in one batch given how they all are interconnected. In fact, this is mostly "extract and move" refactoring. |
|
About the mobile checks to disable the supports, I think that's probably a wrong approach, instead they should update the hooks to "not apply" if needed or apply differently and not disable the support entirely. |
I'll propose that as an independent PR. |
| isDefault: true, | ||
| attributes: { buttonText: __( 'Search' ), label: __( 'Search' ) }, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
That's an interesting usage of "Variations" :)
There was a problem hiding this comment.
I tried the same approach with File block but it uses placeholders so it loses the attribute that is sourced from HTML after save and refresh because it isn't stored with save method.
There are drawbacks in some cases :)
There was a problem hiding this comment.
A note (mostly to myself) for future reference. We could try to experiment with supports: { placeholder: { type: 'media' } }. With that flag, it would show media placeholder and when saving it would serialize attributes sources from HTML as attributes serialized in HTML comment of the block. It would resolve the issue with variations. The same could work with supports: { placeholder: { type: 'variations' } } for Columns block but it would show the variations picker when save returns null.
There was a problem hiding this comment.
Hi! I see this is already approved but I have a few comments/questions about this that I wanted to share:
-
Is it the goal to replace
register_block_typebyregister_block_type_from_metadata? The first is still used in thepost_commentsblock. -
variations for the search block: what if we also added
placeholderfor completeness? I know it's not translated yet so it's not really necessary. It's fine as it is also. -
supports key: besides all the blocks with conditionals you mentioned, there's also the
widget-areablock. Can we migrate this to block.json as well? -
I saw that we added a parent for the
query-loopandquery-paginationblocks. It makes sense to me but also changes the behavior so I wanted to confirm this was intentional. -
We also added
html:falsefor search and post-tags. Not sure what's the criteria for not allowing editing the html of block so wanted to flag to confirm this is intentional.
It's fine by me if we want to do this in follow-up PRs (if they make sense).
Yes, ideally we use
It's an empty string so I think it's fine to leave it as is.
It was added recently, it's hard to keep up but here you go 9ba3e75.
It might be a rebase issue, I will double-check. It was in the JS file before. See 0490313.
I think it's expected since those blocks don't define
I think I addressed all those points :) |
gziolo
force-pushed
the
update/block-json-supports
branch
from
5a39992 to
0490313
Compare
|
I opened #22502 to refactor the remaining blocks listed in #22422 (comment). |
|
Thank you all for help 💯 |
Description
I still consider breaking down this PR into smaller tasks because it contains the following refactorings:
supportstoblock.jsonfiles for core blocksparentstoblock.jsonfiles for core blocksblock.jsonfor the following blocks:server-registered.jsonto use with integration tests and replaces it with the data loaded fromblock.jsonfilesHow has this been tested?
npm run test-unitnpm run test-e2eManually tested that Navigation and Search block works as before.
Types of changes
Refactoring
Checklist: