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

i18n extractor: extract strings from Gutenberg Blocks. #10391

Merged
merged 2 commits into from
Oct 26, 2018

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Oct 24, 2018

Changes proposed in this Pull Request:

This changes the gulp languages:extract task so it can get the strings from any .js files in the _inc/blocks directory. This is currently where we place the Gutenberg Block files generated by the SDK.

In addition, the task now searches for _n(), _x() and _nx(), so we can account for any @wordpress/i18n localization function.

Testing instructions:

  • Checkout the following Calypso branch: remove/gutenberg-jetpack-textdomain - (Calypso PR)
  • Run the following command while in the Calypso root dir:
NODE_ENV=production npm run sdk gutenberg client/gutenberg/extensions/presets/jetpack -- \
--output-dir=/DIR_TO_JETPACK/_inc/blocks \
--watch

where DIR_TO_JETPACK is the directory to your local Jetpack repo.

  • Switch to this branch in Jetpack.
  • Make sure you have gulp globally installed.
  • Run gulp languages:extract
  • Verify _inc/jetpack-strings.php contains new strings, coming from any of the .js block files (towards the end of the file).
  • Start a post.
  • Insert some of our Jetpack blocks, verify they're properly translated.
    • To test this, change the Markdown block title to "Settings" and re-run the above Calypso command. "Settings" is a valid string that's already used by Jetpack and it's translated in many of the locales.

Proposed changelog entry for your changes:

  • None.

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Focus] i18n Internationalization / i18n, adaptation to different languages [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack DO NOT MERGE don't merge it! labels Oct 24, 2018
@jeherve jeherve added this to the 6.7 milestone Oct 24, 2018
@jeherve jeherve requested review from zinigor and a team October 24, 2018 17:12
@jeherve
Copy link
Member Author

jeherve commented Oct 24, 2018

Currently this does not quite work. The strings generated in the _inc/jetpack-strings.php all have the following format:

_n( "Link to", "jetpack", 1, "jetpack" ), // _inc/blocks/editor.js:25

@jetpackbot
Copy link

jetpackbot commented Oct 24, 2018

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS

@tyxla tyxla added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed DO NOT MERGE don't merge it! [Status] In Progress labels Oct 25, 2018
@tyxla
Copy link
Member

tyxla commented Oct 25, 2018

This is now ready for review.

@tyxla tyxla requested review from a team October 25, 2018 11:34
Copy link
Member Author

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It seems to work well in my tests! 🎉

@kraftbj
Copy link
Contributor

kraftbj commented Oct 25, 2018

@jeherve I'll ping in Slack. I'm seeing errors when trying to run through your testing steps, so want to confirm if it is me or the PR. Let's hold on to make sure it is just me.

kraftbj
kraftbj previously approved these changes Oct 25, 2018
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

I had some issues with Calypso due to stale node_modules and in the course of figuring that out, I upgraded node to 11.0.0. The gulp function didn't work as expected under Node 11, but a downgrade to 10.11.0 + rebuild node-sass resolved that.

Once I got the environment working, tests as expected. 🚢

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 25, 2018
jeherve and others added 2 commits October 26, 2018 14:34
- Allow the `gulp languages:extract` task to get the strings from any .js files in the `_inc/blocks`
directory. This is currently where we place the Gutenberg Block files generated by the SDK.
@tyxla tyxla force-pushed the update/gulp-extract-blocks-i18n branch from 8deb5d0 to b9d21d9 Compare October 26, 2018 11:34
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 26, 2018
@tyxla tyxla added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Oct 26, 2018
@tyxla
Copy link
Member

tyxla commented Oct 26, 2018

Folks, this needs another review because we fixed something we missed: Automattic/wp-calypso#28088 (comment)

Note that I've rebased this branch, so you might want to pull the latest rebased version.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 26, 2018
Copy link
Member Author

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It seems to work well in my test. Nice work!

screenshot 2018-10-26 at 16 03 28

@kraftbj kraftbj merged commit d6445e5 into master Oct 26, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 26, 2018
@kraftbj kraftbj deleted the update/gulp-extract-blocks-i18n branch October 26, 2018 19:28
@tyxla
Copy link
Member

tyxla commented Oct 29, 2018

Thanks for landing this one, folks!

Note that the Calypso PR still needs to land before we're able to index translations from blocks: Automattic/wp-calypso#28088

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Focus] i18n Internationalization / i18n, adaptation to different languages [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants