Fix: Register Filter Blocks using block.json
#6505
Conversation
dinhtungdu
added
status: needs review
type: enhancement
rubikuserbot
requested review from
a team and
sunyatasattva
and removed request for
a team
|
Size Change: +657 B (0%) Total Size: 863 kB
ℹ️ View Unchanged
|
| from: './assets/js/blocks/**/block.json', | ||
| to( { absoluteFilename } ) { | ||
| const blockName = absoluteFilename | ||
| .split( '/' ) | ||
| .at( -2 ); | ||
| return `./${ blockName }/block.json`; | ||
| }, | ||
| globOptions: { | ||
| ignore: [ '**/inner-blocks/**' ], | ||
| }, |
There was a problem hiding this comment.
To have a cleaner (and less conflict) folder structure, we might want to pivot away from ${ blockName }/block.json toward ${ blockName }.block.json instead, but I'm not sure how would that play into translation.
ignore: [ '**/inner-blocks/**' ],
I believe lack of having assets in our releases (not confirmed?) and not having them in build is the reason why block metadata translation isn't loading in the editor :/
There was a problem hiding this comment.
I believe lack of having assets in our releases (not confirmed?) and not having them in build is the reason why block metadata translation isn't loading in the editor :/
We only need block.json files in the build to translate the block metadata. asset is unnecessary to get translation working in my test.
we might want to pivot away from ${ blockName }/block.json toward ${ blockName }.block.json instead, but I'm not sure how would that play into translation.
I will test this. But it's possible that make-pot scans only block.json files.
There was a problem hiding this comment.
I will test this. But it's possible that
make-potscans onlyblock.jsonfiles.
Aljullu
left a comment
Aljullu
left a comment
There was a problem hiding this comment.
Thanks for working on this, @dinhtungdu! I have a couple of questions:
Translation for block title and description is handled by
pofile, not JSON.
That means we will break existing translations, right? At least until this version of WC Blocks is merged into WC core and translations are generated again?
make-potdoesn't pick the default value of attributes, so we have to override them on the JS side.
Just curious, couldn't we just declare those attributes in the JS file, then? Or is there any benefit on duplicating them between the JSON and JS files?
@Aljullu Yes.
Yes. I did that so we can grasp all block attributes by looking only at the block.json file. I admit that it also creates confusion at the same time. I don't have a strong opinion on this, I'm happy to delete the duplicated entry in the block.json if it makes more sense. |
Thanks for clarifying! That's unfortunate, do you think we could work around it? One idea would be to:
Do you think that will work?
I don't have a strong opinion either. 😄 I think your solution makes sense, but I might remove the |
…d title and translation to index file to make translation work during the transition phase
|
@Aljullu I address the translation issue per your suggestion. Thanks for that! After consideration, I also remove the duplicated attribute entries because they create more confusion than they solve. |
Aljullu
left a comment
Aljullu
left a comment
There was a problem hiding this comment.
LGTM, thanks for updating the PR, @dinhtungdu!
- Once the translations have been updated, remove the strings from
index.js(we will need to create a follow-up issue for that).
Will you be able to create an issue for this one so we don't forget? ☝️
I created #6543 for this. |
Fixes #6491
This PR:
block.jsonand register the block using metadata for filter blocks.Note:
pofile, not JSON.make-potdoesn't pick the default value of attributes, so we have to override them on the JS side.Accessibility
prefers-reduced-motionOther Checks
Screenshots
N/a
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog