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: Use `wp-components` script handle to pass locale data to `wp.i18n` #5910

Merged
merged 2 commits into from Apr 6, 2018

Conversation

Projects
None yet
3 participants
@ocean90
Member

ocean90 commented Mar 30, 2018

This changes the script handle from wp-edit-post to wp-components for the wp_add_inline_script() call that passes the locale data to wp.i18n.

Fixes #5898.

Testing instructions:

Change language of your WordPress install and check that all components are translated.

@@ -866,7 +866,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
// Prepare Jed locale data.
$locale_data = gutenberg_get_jed_locale_data( 'gutenberg' );
wp_add_inline_script(
'wp-edit-post',
'wp-components',

This comment has been minimized.

@youknowriad

youknowriad Mar 30, 2018

Contributor

I think this doesn't solve the conceptual problem but just moves it to another level. I think the solution here would be to create a separate script for these i18n strings and add it as a dependency to all Gutenberg scripts with translatable strings. I used this technique here https://github.com/youknowriad/gcf/pull/33/files#diff-14f2377adcf109755a6ae6321d6c35afR1

This comment has been minimized.

@ocean90

ocean90 Mar 30, 2018

Member

Why so complicated if you can just use wp-i18n?

This comment has been minimized.

@youknowriad

youknowriad Mar 30, 2018

Contributor

Because if you use wp-i18n you're adding this script to all pages where maybe it's not needed. I guess the ideal is a domain per script :) to avoid loading everything on all pages.

I can be on board with wp-i18n for Gutenberg (because it's Core) but not for plugins though. I also wonder how this would grow over time :).

This comment has been minimized.

@ocean90

ocean90 Mar 30, 2018

Member

Mhh, you only enqueue and add inline scripts when the page needs them. Just because plugin X adds the translations for page Y, it doesn’t mean that the translations are automatically available on all pages.

This comment has been minimized.

@youknowriad

youknowriad Mar 30, 2018

Contributor

It doesn't make sense to load the wp-components scripts in any page without the translated strings it depends on.

It doesn't make sense to load the wp-editor scripts in any page without the translated strings it depends on.

If a plugin author creates a new page in WP Admin and is depending on wp-components because it's using one of these generic components. The only thing this plugin author should do is add a dependency on wp-components to its scripts where these components are used, the plugin author shouldn't have to worry about setting translated strings for this module, this is just implementation detail of the wp-components script.

So this means the translated strings are just a dependency for the wp-components like any other dependency and it should be defined at registration time and not when enqueuing the scripts.

If you decide to add this inline script at enqueue time, you'll have to do it on each page this script is loaded in.

This comment has been minimized.

@youknowriad

youknowriad Mar 30, 2018

Contributor

Again, that's the theory, I'm fine with making a compromise for Gutenberg translation strings (or Core JS translation strings in general) and just always load them in all pages. (which means adding them to wp-i18n at registration time)

This comment has been minimized.

@ocean90

ocean90 Mar 31, 2018

Member

I'm not really following you logic here.
wp-components, wp-blocks, wp-editor and wp-edit-post are currently the only scripts that depend on wp-i18n . Since the strings are not separated and based on the dependency tree, wp-components is the common script used by all of them and therefore should be used to register the translations. Just like in this PR demonstrated.

How scripts and translations are registered with Gutenberg in core is another topic and should probably be part of the merge proposal.

This comment has been minimized.

@youknowriad

youknowriad Mar 31, 2018

Contributor

wp-components is the common script used by all of them and therefore should be used to register the translations. Just like in this PR demonstrated.

That's true now but that's just a coincidence, what if we introduce a new Gutenberg module that doesn't depend on components but still requires i18n, what if we add a translated string to wp-data in another PR? I'd be more confident (more future proof) if we add this as to dependency of wp-i18n

'wp.i18n.setLocaleData( ' . json_encode( $locale_data ) . ' );',
'before'
'wp-i18n',
'wp.i18n.setLocaleData( ' . json_encode( $locale_data ) . ' );'

This comment has been minimized.

@youknowriad

youknowriad Apr 1, 2018

Contributor

Should we move this inline script to line 140 (registration time)?

This comment has been minimized.

@ocean90

ocean90 Apr 4, 2018

Member

No, because gutenberg_editor_scripts_and_styles() gets only called if Gutenberg gets initialized while gutenberg_register_scripts_and_styles() is called on all pages. Also, the strings are not used by wp-i18n.

This comment has been minimized.

@youknowriad

youknowriad Apr 4, 2018

Contributor

That's exactly the point I was raising before, what if a plugin adds a dependency to wp-components or wp-blocks in a separate page? The strings should be loaded without any other adjustment from the plugin author.

This comment has been minimized.

@ocean90

ocean90 Apr 4, 2018

Member

It's a fundamental problem with the current API. Based on WordPress/packages#96 (comment), "there's no blocking here. So moving forward.". 🤷‍♂️

This comment has been minimized.

@youknowriad

youknowriad Apr 4, 2018

Contributor

@aduth Thoughts on this

This comment has been minimized.

@aduth

aduth Apr 4, 2018

Member

It will need further changes for merge. I imagine it might be the domain-per-script idea you already mentioned, ideally with some automation for assigning domain to a script bundle for string extraction and injecting the locale data inline script. Admittedly unclear how well this fits into existing localization workflows.

As for what to do here, I don't feel strongly against this implementation as a short-term solution, but I agree the locale data should ideally be set as a side effect of depending on any of the internationalized Gutenberg scripts. I see this as a stubbed script upon which all of the Gutenberg bundles depend, with the inline script to set locale data. This was already mentioned, though dismissed as complicated. Personally it doesn't seem very complicated to me 🤷‍♂️

This comment has been minimized.

@youknowriad

youknowriad Apr 4, 2018

Contributor

I imagine it might be the domain-per-script idea you already mentioned

Just noting that this is impossible to do because the translate.wp.org workflow forces the domain used in the plugin to match the plugin slug AFAIK

@ocean90 ocean90 added this to the 2.6 milestone Apr 4, 2018

@youknowriad youknowriad modified the milestones: 2.6, 2.7 Apr 5, 2018

@ocean90

This comment has been minimized.

Member

ocean90 commented Apr 6, 2018

We now have #6015 which addresses the remaining concerns. Since this PR was/is only supposed to fix the issue mentioned in #5898 I'm going to merge it so it doesn't miss another release.

Having something translated is more important for translators/testers than trying to figure out how the translations are handled under the hood. The second point can be done in the background (and not hidden in a PR) while visually everything looks fine.

@ocean90 ocean90 merged commit a5a9465 into WordPress:master Apr 6, 2018

2 checks passed

codecov/project 44.17% remains the same compared to d78163d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ocean90 ocean90 deleted the ocean90:fix/component-translations branch Apr 6, 2018

@aduth

This comment has been minimized.

Member

aduth commented Apr 6, 2018

The second point can be done in the background (and not hidden in a PR) while visually everything looks fine.

How can we plan to assure this will happen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment