Skip to content
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

Gutenberg: Add i18n wrappers to provide the Jetpack textdomain #28088

Merged
merged 3 commits into from
Oct 29, 2018

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Oct 25, 2018

It seems we'll have to ditch the textdomain from the i18n function calls in our blocks for good. Reason: it messes with how xgettext-js parses the files.

However, we need to still provide the textdomain so Gutenberg would be able to find the Jetpack translations where they are.

To address this, we provide wrappers for the wp.i18n functions that add the textdomain, and by this:

  • Gutenberg will still have the textdomain at the time the i18n function is called.
  • Jetpack will not have the textdomain, as it will index our wrapper functions instead, so indexing will work well.

Changes proposed in this Pull Request

  • Remove jetpack textdomain from any Jetpack blocks in order to allow Jetpack to index the translations properly.
  • Introduce wrappers for @wordpress/i18n l10n functions.
  • Use i18n wrappers instead of @wordpress/i18n

Testing instructions

@tyxla tyxla added Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. i18n [Goal] Gutenberg Working towards full integration with Gutenberg labels Oct 25, 2018
@tyxla tyxla self-assigned this Oct 25, 2018
@matticbot
Copy link
Contributor

@ockham
Copy link
Contributor

ockham commented Oct 25, 2018

It seems we'll have to ditch the textdomain froIt seems we'll have to ditch the textdomain from the i18n function calls in our blocks for good. Reason: it messes with how xgettext-js parses the files.

Note, we've been discussing (pa9srA-W-p2) using the @wordpress/babel-plugin-makepot (which is already used by Gutenberg) instead of xgettext-js.

/cc @akirk

@tyxla
Copy link
Member Author

tyxla commented Oct 25, 2018

Note, we've been discussing (pa9srA-W-p2) using the @wordpress/babel-plugin-makepot (which is already used by Gutenberg) instead of xgettext-js.

If this can be a quick change, that sounds good, but if it takes a while, FWIW it will be a blocker for shipping localized and translatable Jetpack blocks.

@tyxla
Copy link
Member Author

tyxla commented Oct 26, 2018

Well, it seems that if we remove textdomain, we'll remove the possibility for wp.i18n to find translations...

I'll try with a different solution, likely with our wrappers for the wp.i18n localization functions.

@tyxla tyxla changed the title Gutenberg: Remove jetpack textdomain from all blocks Gutenberg: Add i18n wrappers to provide the Jetpack textdomain Oct 26, 2018
@tyxla
Copy link
Member Author

tyxla commented Oct 26, 2018

I'll try with a different solution, likely with our wrappers for the wp.i18n localization functions.

Done, this is ready for another review.

@tyxla tyxla added the [Pri] High Address as soon as possible after BLOCKER issues label Oct 29, 2018
@tyxla
Copy link
Member Author

tyxla commented Oct 29, 2018

Placing it under [Pri] High as this needs to land soon to allow Jetpack blocks to be translatable.

@tyxla tyxla force-pushed the remove/gutenberg-jetpack-textdomain branch from 27aac77 to 7b0898d Compare October 29, 2018 11:35
@tyxla
Copy link
Member Author

tyxla commented Oct 29, 2018

Also, rebased.

@lezama
Copy link
Contributor

lezama commented Oct 29, 2018

let's merge it!

@lezama lezama added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 29, 2018
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in Calypso-Gutenberg: works well apart from expected "Jed localization error: Error: Domain jetpack was not found."

Tested in Jetpack-Gutenberg: works great, tested changing block title to "Settings" and I could see it translated to Finnish.

Tested exporting translation strings in Jetpack by npx gulp languages:extract — I can see the script looking into block files but I didn't see it pick any strings to _inc/jetpack-strings.php file in Jetpack. Was this expected?

Noting also that the build bundle fails in latest Jetpack master and latest Gutenberg with some lodash error, but that seems unrelated to this PR. I removed all the other blocks except Markdown from the preview and this worked great. 👍

Code looks great, clever idea!

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested exporting translation strings in Jetpack by npx gulp languages:extract — I can see the script looking into block files but I didn't see it pick any strings to _inc/jetpack-strings.php file in Jetpack. Was this expected?

Tested again by building minified prod-versions by adding NODE_ENV=production to SDK command and now extracting strings works like a charm. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg i18n Jetpack [Pri] High Address as soon as possible after BLOCKER issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants