Blocks: Introduce registerBlockTypeFromMetadata API #30293
Conversation
gziolo
added
[Status] In Progress
|
Size Change: +647 B (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
gziolo
force-pushed
the
add/register-block-type-from-metadata
branch
3 times, most recently
from
dd78fb3 to
1a6154e
Compare
gziolo
force-pushed
the
add/register-block-type-from-metadata
branch
from
1a6154e to
a0bba9e
Compare
|
It's nearly ready. I still have to find out how to dynamically translate the value read from the |
gziolo
added
the
Needs Dev Note
gziolo
force-pushed
the
add/register-block-type-from-metadata
branch
from
a0bba9e to
c0b328d
Compare
gziolo
force-pushed
the
add/register-block-type-from-metadata
branch
from
c0b328d to
7f6d4cb
Compare
gziolo
force-pushed
the
add/register-block-type-from-metadata
branch
from
7f6d4cb to
d9ba0cb
Compare
| textdomain | ||
| ) { | ||
| if ( isString( i18nSchema ) && isString( settingValue ) ) { | ||
| // eslint-disable-next-line @wordpress/i18n-no-variables, @wordpress/i18n-text-domain |
There was a problem hiding this comment.
@swissspidy, do we need to do something special to ensure this particular translation call is ignored during WP-CLI processing?
There was a problem hiding this comment.
String extraction will skip anything that‘s not a string literal, so no.
There was a problem hiding this comment.
@gziolo How are the translations supposed to be in wp.i18n without a specific wp.i18n.setLocaleData() call? The strings from the block.json aren't assigned to a JavaScript file thus not automatically part of a JSON translation file. But I might be missing something here? Or are we fully relying on server-side registration for the strings?
There was a problem hiding this comment.
In the context of WordPress core, they are always registered on the server and translated there before passed to the client. At the moment, there is a temporary JS method unstable__bootstrapServerSideBlockDefinitions that takes care of setting data generated with PHP. In the future, we are going to use the REST API endpoint for blocks, but the general idea is going to be the same - translations get applied when registering a block on the server.
In practice, this implementation is a fallback for everything else that depends only on JavaScript:
- standalone block editor like Storybook integration (although this is a bad example as it is never translated)
- native mobile apps still consume only JavaScript codebase so they have to collect all translations themselves (it might change when we start using the REST API endpoint for blocks)
| @@ -0,0 +1,12 @@ | |||
| { | |||
There was a problem hiding this comment.
@gwwar, should we put here variations as well which we plan to integrate with block.json soon?
There is one more place where we would need to put it on the allow list.
There was a problem hiding this comment.
I think it'd be worth a try, will it affect the needed PR order?
There was a problem hiding this comment.
Let me think about what we need to change to enable variations in block.json on the PHP side. As far as I can tell, it's mostly the same set of changes like here now that WP_Block_Type and the REST API endpoint support block variations. Given that you won't be able to register block variations on the server until WP 5.8 is out the order shouldn't matter at all. We also have a mechanism in place that favors definitions from the server but fallback to the client when not set there. I think it's safe to change here at the same time 👍🏻
There was a problem hiding this comment.
Added in b851c2f. Every change to docs get synced with https://developer.wordpress.org/block-editor/reference-guides/block-api/block-metadata/ so I will open another PR that covers the part that describes variations in the block.json file. We should plan to merge it around WordPress 5.8 RC.1 when all Dev Notes get published.
|
I opened #31120 that is based on this branch and it presents how it will simplify handling blocks: diff --git a/packages/block-library/src/archives/block.json b/packages/block-library/src/archives/block.json
index d35f8b05001c..0c94c4b44b35 100644
--- a/packages/block-library/src/archives/block.json
+++ b/packages/block-library/src/archives/block.json
@@ -1,7 +1,10 @@
{
"apiVersion": 2,
"name": "core/archives",
+ "title": "Archives",
+ "description": "Display a monthly archive of your posts.",
"category": "widgets",
+ "textdomain": "default",
"attributes": {
"displayAsDropdown": {
"type": "boolean",
diff --git a/packages/block-library/src/archives/index.js b/packages/block-library/src/archives/index.js
index 6bfdbef7abce..e1f01f30a718 100644
--- a/packages/block-library/src/archives/index.js
+++ b/packages/block-library/src/archives/index.js
@@ -2,7 +2,6 @@
* WordPress dependencies
*/
import { archive as icon } from '@wordpress/icons';
-import { __, _x } from '@wordpress/i18n';
/**
* Internal dependencies
@@ -15,8 +14,6 @@ const { name } = metadata;
export { metadata, name };
export const settings = {
- title: _x( 'Archives', 'block title' ),
- description: __( 'Display a monthly archive of your posts.' ),
icon,
example: {},
edit, |
gziolo
force-pushed
the
add/register-block-type-from-metadata
branch
from
b851c2f to
f371ba8
Compare
| 'icon', | ||
| 'description', | ||
| 'keywords', | ||
| 'attributes', |
There was a problem hiding this comment.
Does i18n support get added for string default values? For example let's say we have
"attributes": {
"label": {
"type": "string",
"default": "Home",
},
"opensInNewTab": {
"type": "boolean",
"default": false,
},
}
There was a problem hiding this comment.
No, we should rather avoid it because it won't produce predictable content. Depending on the language picked in the admin UI, you could get a different output for the same block. We need to remember also that, the value of an attribute when strictly equal to the default value it doesn't get stored in HTML comment of the block definition. The other challenge is, how would you decide which default values need to be translated and for which blocks?
There was a problem hiding this comment.
We need to remember also that, the value of an attribute when strictly equal to the default value it doesn't get stored in HTML comment of the block definition.
🤔 good to highlight this case.
The other challenge is, how would you decide which default values need to be translated and for which blocks?
If we wanted to support it, we could add an additional attribute. I found that I did need it in one of the blocks, but the behavior can also be simulated with a useEffect hook in the block.
There was a problem hiding this comment.
If we wanted to support it, we could add an additional attribute. I found that I did need it in one of the blocks, but the behavior can also be simulated with a
useEffecthook in the block.
When using a side effect as part of the React component lifecycle, you are updating an attribute that might create an undo level so you need to use a special version of the store action that mitigates that. Another approach is to use a block variation that gets applied as the default one in the inserter. The benefit of that is that it sets attributes dynamically when inserting the block.
|
Let us know what type of feedback/testng you'd like on this one @gziolo to help move this forward. |
| ) { | ||
| if ( isString( i18nSchema ) && isString( settingValue ) ) { | ||
| // eslint-disable-next-line @wordpress/i18n-no-variables, @wordpress/i18n-text-domain | ||
| return _x( settingValue, i18nSchema, textdomain ); |
There was a problem hiding this comment.
Aren't we going to cause a string extraction because of the usage of _x function? Maybe because we are using variables the extraction does not happen? On the PHP side, we use an internal function to avoid this issue.
There was a problem hiding this comment.
See comment from @swissspidy in #30293 (comment):
String extraction will skip anything that‘s not a string literal
| ) { | ||
| if ( isString( i18nSchema ) && isString( settingValue ) ) { | ||
| // eslint-disable-next-line @wordpress/i18n-no-variables, @wordpress/i18n-text-domain | ||
| return _x( settingValue, i18nSchema, textdomain ); |
There was a problem hiding this comment.
Things like the block description currently don't have a context and now are going to have one. I guess their current translation will be invalidated, should we make some dev note or something equivalent to dev note for translators?
There was a problem hiding this comment.
As far as I understand it will be automatically handled during the WordPress release cycle, and it's the same process that happens for every newly added or updated translation. We did a batch updated of UI labels in the block editor in the past, and I don't remember any dev notes related to that.
| ) { | ||
| if ( isString( i18nSchema ) && isString( settingValue ) ) { | ||
| // eslint-disable-next-line @wordpress/i18n-no-variables, @wordpress/i18n-text-domain | ||
| return _x( settingValue, i18nSchema, textdomain ); |
There was a problem hiding this comment.
In the branch of PR #31120, I made a simple test and replaced "return _x( settingValue, i18nSchema, textdomain );" with "return 'ok';". Nothing changed. With this change shouldn't all the block titles and descriptions appear as "ok"?
There was a problem hiding this comment.
It might require adding a fallback to read the value from the client when it isn't provided from the server. It would be useful for WordPress 5.7 when using with the Gutenberg plugin.
There was a problem hiding this comment.
I investigate it further and it's a different use case than apiVersion setting that I was referring to:
gutenberg/packages/blocks/src/api/registration.js
Lines 167 to 181 in c047e27
We register all blocks on the server and we set now all translatable fields there and they are exposed to the client. The values from the server take precedence to respect all PHP filters that could get applied to the metadata. It's the same data after all, so there is no reason to override it with the same metadata read on the client.
To test it, you could change the method I embedded above and enforce that those fields get overridden with the values loaded on the client. Maybe you could also tweak register_block_type_from_metadata calls on the server to somehow ignore translatable fields using block_type_metadata_settings filter, but you rather would need to prevent registration of the block you want to test with on the server.
There was a problem hiding this comment.
Thank you for the clarification 👍
I opened #31120 which is based on this branch that moves all translatable fields to |
|
|
||
| const settings = pick( metadata, allowedFields ); | ||
|
|
||
| if ( textdomain ) { |
There was a problem hiding this comment.
As far as I checked, in the native version we set the locale data without defining a text domain, not sure if this could be a problem if in the future we move some of the fields to the block.json file to do the on fly translation.
There was a problem hiding this comment.
default is the default text domain, so this is what I'm using in #31120. I think it should be fine.
youknowriad
removed
the
Needs Dev Note
Description
Fixes #23636.
This PR proposes we introduce a new API method
registerBlockTypeFromMetadatathat processes metadata loaded fromblock.jsonand applies translations on the fly to the corresponding fields.Dev Note
In JavaScript, you can use now
registerBlockTypeFromMetadatamethod from@wordpress/blockspackage to register a block type using the metadata loaded fromblock.jsonfile. All localized properties get automatically wrapped in_x(from@wordpress/i18npackage) function calls similar to how it works in PHP withregister_block_type_from_metadata. The only requirement is to set thetextdomainproperty in theblock.jsonfile.Example:
block.json{ "name": "my-plugin/translatable-block", "title": "My translatable block", "description": "My block with translatable metadata", "keywords": [ "translatable" ], "textdomain": "my-plugin" }index.jsHow has this been tested?
npm run test-unitTypes of changes
New API.
Checklist:
*.native.jsfiles for terms that need renaming or removal).