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

Add text domain to copy in domain-picker and plans-grid packages #46557

Merged
merged 8 commits into from
Oct 23, 2020

Conversation

p-jackson
Copy link
Member

@p-jackson p-jackson commented Oct 19, 2020

Changes proposed in this Pull Request

  • Define __i18n_text_domain__
  • Hard code __i18n_text_domain__ to 'default' in calypso
  • Hard code __i18n_text_domain__ to 'full-site-editing' in ETK
  • Hard code __i18n_text_domain__ to some value in the unit tests so they don't fail
  • Switch @automattic/domain-picker and @automattic/plans-grid over to use @wordpress/18n
  • Add the text-domain constant to all calls to __() in the domain-picker and plans-grid packages

Testing instructions

  • Check out branch and test using yarn dev --sync
  • Switch user to Spanish or something non-English
  • Go to /new/domains and see that the "View more results" button and other copy is still translated correctly
  • Open the page editor on an unlaunched gutenboarding site
  • Click "Complete setup" in the top right
  • Click the second step in the launch flow, which is for choosing a domain
    • The text in here is being translated, but will probably still appear as English because translations aren't done for the ETK (the strings are translated for the default text-domain, but not the full-site-editing text-domain)
    • You can confirm the translations are working by switching one of the strings in the domain picker with something that we know is already translated in the full-site-editing text domain
    • e.g. Switch "Search for a domain" with "Complete setup", rebuild, and the domain search placeholder text should be translated.

@p-jackson p-jackson added i18n [Goal] New Onboarding previously called Gutenboarding labels Oct 19, 2020
@p-jackson p-jackson requested a review from a team October 19, 2020 05:58
@p-jackson p-jackson self-assigned this Oct 19, 2020
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 19, 2020
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D51378-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

@matticbot
Copy link
Contributor

matticbot commented Oct 19, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~9 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding       +695 B  (+0.0%)       +9 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Collaborator

@wp-desktop wp-desktop left a comment

Choose a reason for hiding this comment

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

WordPress Desktop CI Failure for job "wp-desktop-mac".

@p-jackson please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

@ramonjd ramonjd added the [Pri] High Address as soon as possible after BLOCKER issues label Oct 19, 2020
@p-jackson p-jackson force-pushed the add/text-domain-to-domain-picker-copy branch from dd09f22 to b516b39 Compare October 19, 2020 21:21
@wp-desktop wp-desktop dismissed their stale review October 19, 2020 21:46

wp-desktop ci passing, closing review

@p-jackson p-jackson force-pushed the add/text-domain-to-domain-picker-copy branch from cca05d9 to 73932ea Compare October 19, 2020 22:40
@p-jackson
Copy link
Member Author

This approach is probably ready for a review, but added [DO NOT MERGE] because there's an issue with very early domain suggestion searches using the wrong locale. Need to see whether this is introduced by the PR or already an issue in production.

@p-jackson p-jackson marked this pull request as ready for review October 20, 2020 01:22
@p-jackson p-jackson force-pushed the add/text-domain-to-domain-picker-copy branch from baae97e to 329abec Compare October 20, 2020 04:07
@p-jackson
Copy link
Member Author

there's an issue with very early domain suggestion searches using the wrong locale. Need to see whether this is introduced by the PR or already an issue in production.

This issue already exists in production. Unrelated to this PR but, if user searches for a domain before the translations are loaded then the request to the domains/suggestions will include a locale=en param, even if the locale has been set to something else.

This is fixed by #46473

Gutenboarding translation already works with @wordpress/i18n
Webpack supports having multiple DefinePlugins in the build, so we don't
need to worry about canceling out the affect of the other DefinePlugin
that's already provided by the shared webpack config.
@p-jackson p-jackson force-pushed the add/text-domain-to-domain-picker-copy branch from 71b69f0 to 7644403 Compare October 22, 2020 01:30
@p-jackson p-jackson changed the title Add text domain to copy in domain-picker package Add text domain to copy in domain-picker and plan-grid packages Oct 22, 2020
@p-jackson p-jackson changed the title Add text domain to copy in domain-picker and plan-grid packages Add text domain to copy in domain-picker and plans-grid packages Oct 22, 2020
@lsl
Copy link
Contributor

lsl commented Oct 22, 2020

Does string extraction handle these constant text domains correcty?


This issue already exists in production. Unrelated to this PR but, if user searches for a domain before the translations are loaded then the request to the domains/suggestions will include a locale=en param, even if the locale has been set to something else.

This is fixed by #46473

Is this not the hardcoding of en locale here: #44840 (comment)

Copy link
Contributor

@deBhal deBhal left a comment

Choose a reason for hiding this comment

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

LGTM!

It looks like it's fixed our problem - I spun up an ephemeral site, ran yarn run build for calypso and the etk, and then copied the new editor-site-launch.js bundle across to run wp i18n make-pot . there. I also made a pot against master for comparison with the original pot from the provisioning, and the diffs are just what we want to see (pasted below).

I also checked that we haven't messed with the calypso string extraction by running yarn run translate on this and the merge base (git mergebase HEAD master) and diffing them, and saw only some line-number changes in domain-picker and plans-grid - perfect 👍

I'd like to keep looking for a cleaner solution in general, but this here is a working patch - nice work! :shipit:

[email protected]:~/wp-content/plugins/full-site-editing$ diff full-site-editing.pot.orig full-site-editing.pot.master
< "POT-Creation-Date: 2020-10-21T01:42:14+00:00\n"
---
> "POT-Creation-Date: 2020-10-23T00:40:38+00:00\n"
[email protected]:~/wp-content/plugins/full-site-editing$ diff full-site-editing.pot.orig full-site-editing.pot.patched 
12c12
< "POT-Creation-Date: 2020-10-21T01:42:14+00:00\n"
---
> "POT-Creation-Date: 2020-10-23T00:25:10+00:00\n"
1691a1692,1755
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "All domains ending with <tld /> require an SSL certificate to host a website. When you host this domain at WordPress.com an SSL certificate is included. <learn_more_link>Learn more</learn_more_link>"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "Recommended"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "You can change your free subdomain later under Domain Settings."
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "Unavailable"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "Free"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "Included in plans"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "%s/year"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "All Categories"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "View all"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "example.com"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "Search for a domain"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "An error has occurred, please check your connection and retry."
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "Keep sub-domain"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "Professional domains"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "View more results"
> msgstr ""
> 
> #: editor-site-launch/dist/editor-site-launch.js:33
> msgid "A domain name is the site address people type in their browser to visit your site."

@p-jackson
Copy link
Member Author

Is this not the hardcoding of en locale here: #44840 (comment)

@lsl yes it is. The locale still defaults to 'en'. We attempt to pass in the correct locale using the useI18n() hook, but until the locale data has been loaded, the hook doesn't return the correct locale.

However now that #46473 is merged, the domain picker component doesn't render until after the locale data is available, which means the first time we call useI18n() it returns the correct thing.

So not a very explicit fix to the problem, but something for another PR.

@ramonjd ramonjd mentioned this pull request Oct 23, 2020
46 tasks
@p-jackson
Copy link
Member Author

I'd like to keep looking for a cleaner solution in general, but this here is a working patch

I agree, our work here is not done. I was hesitant to merge in case we want to have more discussion about other approaches. But I also don't want this PR to languish over the weekend while I'm afk, slowing our momentum. So I'm going to merge. We absolutely still may want to make changes here.

@p-jackson p-jackson merged commit 3e020d3 into master Oct 23, 2020
@p-jackson p-jackson deleted the add/text-domain-to-domain-picker-copy branch October 23, 2020 05:01
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] New Onboarding previously called Gutenboarding i18n [Pri] High Address as soon as possible after BLOCKER issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants