1171: Ensure translatable strings in blocks can actually be translated #1173

miina · 2018-05-24T07:21:58Z

Add creating amp.pot file from blocks' JS files and add to inline script for making sure that the strings can be translated.

Fixes #1171.

miina · 2018-05-24T07:24:33Z

languages/amp.pot

+msgid ""
+msgstr ""
+"Content-Type: text/plain; charset=utf-8\n"
+"X-Generator: babel-plugin-makepot\n"


@westonruter Do you think that this file should be fully under version control or created when building? Looks like the file has to exist at least being empty for not creating missing file error when building, currently it's just an almost empty file, should we add all the strings under version control?

I think we should follow Gutenberg's lead here. The POT file is not committed to version control: https://github.com/WordPress/gutenberg/tree/master/languages

The POT file generation should be done as part of the build.

miina · 2018-05-24T07:26:37Z

includes/admin/class-amp-editor-blocks.php

-		wp_add_inline_script( 'amp-editor-blocks', sprintf( 'ampEditorBlocks.boot();' ) );
+		wp_add_inline_script(
+			'amp-editor-blocks',
+			'wp.i18n.setLocaleData( ' . wp_json_encode( gutenberg_get_jed_locale_data( 'amp' ) ) . ', "amp" );' . sprintf( 'ampEditorBlocks.boot();' ),


At this moment it's just including the amp.pot file created by babel, not including the PHP file translations, wondering if we should create a separate domain for the blocks and load only that specific file within the blocks instead, e.g. amp-js?

That's one of the questions raised in https://make.wordpress.org/core/2018/05/01/javascript-internationalization-the-missing-pieces/ under “Loading only specific set of translations”.

Use a different text domain per JavaScript module. Export a JSON file per text domain. This is appealing, but has to be ruled out quickly: the text domain is not known to GlotPress and is not stored in the database or anything.

If Babel is just creating an amp.pot for the JS strings, then we can just use that as-is and ignore the amp.pot for all other purposes. In other words, maybe there could be an alternative to gutenberg_get_jed_locale_data() which allows you to pass a path to a POT file instead (or maybe there is an underlying function that could be used instead). And then this POT file would not be used for translating the plugin as a whole. Instead, Babel should generate a dummy PHP file that contains all of the strings in it for translation by GlotPress. You can see that Gutenberg does this by generating a languages/gutenberg-translations.php file in addition to the languages/gutenberg.pot.

Not sure if this is possible, but it's what I'd be looking into.

If I understand correctly then the following is true on using different text domain per JS module, however, it should be possible for separating the editor blocks translation file for all the blocks from the rest in case of this plugin since babel is creating the .pot file based on the block directory files only.

Use a different text domain per JavaScript module. Export a JSON file per text domain. This is appealing, but has to be ruled out quickly: the text domain is not known to GlotPress and is not stored in the database or anything.

Added a commit where amp-js locale is registered for editor blocks JS translation and is creating a separate amp-js.pot file for translation of the blocks, leaving amp.pot available for PHP and any other translations outside of blocks directory: 8ac7759. At least it seems to be working locally when translating some block-related strings within amp-js-et.po and strings from PHP files within amp-et.po.

Do you think this could be suitable setup for the plugin?

I don't know how well this will work with GlotPress on translate.wordpress.org. My understanding is that the POT files are not read and that only the strings used in PHP files are parsed and made available for translation. How would GlotPress be told of the POT files in the first place? I could very well be wrong. @swissspidy Do you have insights to share on the approach?

Essentially:

as of now translate.wordpress.org ignores any POT files in a plugin and only scans PHP files

Whatever strings you extract from JS files and save in a POT file, you need to write to a PHP file so translate.wordpress.org finds them. Just like Gutenberg does it.

For that PHP file you use the same text domain as in the rest of the plugin („amp“)

The text domain (not locale! That‘s a completely different thing) in the JS file basically doesn‘t matter because you only need it for creating that PHP file. For consistency I would still use „amp“

JS I18N in WordPress is being shaped/defined right now. Loading only some strings for a particular JS module might make sense for a large plugin like Gutenberg, but not for this one IMO. It‘s better to wait for official recommendations and implementation.

Even if creating different PO files works locally, it doesn‘t matter if your plugin is hosted on WordPress.org. WordPress.org provides a single translation for all of your plugin‘s strings.

Again, this stuff is being figured out at the moment, so please join us in our efforts to standardize this for core and plugins.

Regarding telling about POT files: it‘s one of the possible features for JS 18N, see our latest chat summary.

For reference: https://github.com/swissspidy/gutenberg-i18n-block

Good to know that the JS i18n defining is happening right now, taking a look at the chat summary. Thanks for the clarification about the limitations of the current system.

swissspidy

Overall this looks good to me. As mentioned, JS I18N is pretty much in flux, so there will be some changes and improvements in the future. For now, this is probably as good as one can do in WP :-)

westonruter · 2018-05-29T17:45:54Z

package.json

@@ -30,7 +32,9 @@
 },
 "main": "blocks/index.js",
 "scripts": {
-  "build": "cross-env BABEL_ENV=production webpack; grunt build",
+  "pot-to-php": "pot-to-php languages/amp-js.pot languages/amp-translations.php amp",
+  "makepot": "wp i18n make-pot .",


I'm having a very difficult time getting this to run. I had to update my WP-CLI to install the i18n package, but now running it exits with error code 137. This seems to indicate that the VM runs out of memory while running. So I had to bump VVV's memory usage from 2048MB to 4096MB. Then I can get it to run, but it takes an agonizing 1+ minute to run. @swissspidy Is this normal?

* Avoid running translation logic when creating normal builds. * Add Grunt task for invoking Webpack. * Add build-release command for creating a ZIP with translations.

swissspidy · 2018-05-29T19:59:29Z

contributing.md

+npm run build-release
+```
+
+Note that this will currently take much longer than a regular build because it generates the files required for translation. You also must have WP-CLI installed with the `i18n` package.


@westonruter You might wanna add a link to https://github.com/wp-cli/i18n-command

Regarding the performance: a quick guess is that the command currently scans every folder looking for PHP files to parse—including node_modules. We might wanna exclude this directory (and others) by default. See https://github.com/wp-cli/i18n-command/blob/90f3f8e80a8e7db71e3b1e2d2b26140d5dd9e9fe/src/WordPressCodeExtractor.php#L115-L135 and wp-cli/i18n-command#27.

If you try running wp i18n make-pot without a node_modules folder being there, it should definitely be very fast. If not, it's something else.

@swissspidy I tried moving vendor and node_modules out of the directory without much improvement. But when I moved .git out of the directory, then it became much faster. It seems that it's scanning all the files inside the .git directory as well. So yeah, excluding those directories seems like the solution!

miina added 2 commits May 24, 2018 10:15

Ensure JS strings can be translated.

437bf32

Fix translation file name.

c967dee

miina commented May 24, 2018

View reviewed changes

miina added 2 commits May 24, 2018 22:11

Add separate locale for blocks.

8ac7759

Create .php translation file from JS translations.

5275055

swissspidy approved these changes May 29, 2018

View reviewed changes

westonruter approved these changes May 29, 2018

View reviewed changes

westonruter added this to the v1.0 milestone May 29, 2018

westonruter reviewed May 29, 2018

View reviewed changes

Make sure translations are included in release builds

cc636ce

* Avoid running translation logic when creating normal builds. * Add Grunt task for invoking Webpack. * Add build-release command for creating a ZIP with translations.

westonruter merged commit 76b929c into develop May 29, 2018

westonruter deleted the add/1171-support_blocks_string_translation branch May 29, 2018 18:48

swissspidy reviewed May 29, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1171: Ensure translatable strings in blocks can actually be translated #1173

1171: Ensure translatable strings in blocks can actually be translated #1173

miina commented May 24, 2018 •

edited

Loading

miina May 24, 2018

westonruter May 24, 2018

miina May 24, 2018 •

edited

Loading

westonruter May 24, 2018

miina May 24, 2018

westonruter May 24, 2018

swissspidy May 24, 2018

swissspidy May 24, 2018

swissspidy May 24, 2018

miina May 25, 2018

swissspidy left a comment

westonruter May 29, 2018

swissspidy May 29, 2018

swissspidy May 29, 2018 •

edited

Loading

westonruter May 29, 2018

1171: Ensure translatable strings in blocks can actually be translated #1173

1171: Ensure translatable strings in blocks can actually be translated #1173

Conversation

miina commented May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swissspidy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swissspidy May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina commented May 24, 2018 •

edited

Loading

miina May 24, 2018 •

edited

Loading

swissspidy May 29, 2018 •

edited

Loading