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

Onboarding: Onboarding related packages use react-i18n to translate copy #48232

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

p-jackson
Copy link
Member

@p-jackson p-jackson commented Dec 10, 2020

Changes proposed in this Pull Request

Copy in the new onboarding flow wasn't updating when switching languages. This happened because we switched onboarding over to use @wordpress/i18n as a temporary work around while react-i18n wasn't working in ETK. Now that react-i18n works in ETK we can use it everywhere, meaning components will re-render when the language is switched.

  • Onboarding related packages switched to use @automattic/react-i18n instead of @wordpress/i18n

There's also an issue with the plan grid text that I was going to fix as part of this PR. But I think that one is going to need a bit of a refactor of the plans data store, so will do that in another PR.

Testing instructions

  • /new?flags=gutenboarding/language-picker
  • Switch language on each step of onbaoarding and confirm that all the copy updates
  • There's an issu on the plans step where the copy in the table doesn't update (see above) however the copy surrounding the table should update
  • cd apps/editing-toolkit && yarn dev --sync and check that all the steps in the launch flow are still translated

Fixes #48196

@p-jackson p-jackson added [Type] Bug i18n [Goal] New Onboarding previously called Gutenboarding labels Dec 10, 2020
@p-jackson p-jackson self-assigned this Dec 10, 2020
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Dec 10, 2020

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

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

name                 parsed_size           gzip_size
entry-gutenboarding       -267 B  (-0.0%)      +71 B  (+0.0%)

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

Async-loaded Components (~62 bytes added 📈 [gzipped])

name                                           parsed_size           gzip_size
async-load-calypso-blocks-editor-launch-modal       -267 B  (-0.1%)      +62 B  (+0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

@p-jackson p-jackson mentioned this pull request Dec 11, 2020
46 tasks
@lsl
Copy link
Contributor

lsl commented Dec 14, 2020

Was this the thing we discussed updating i18n data in the useI18n hook so that you could use either?

@p-jackson
Copy link
Member Author

Was this the thing we discussed updating i18n data in the useI18n hook so that you could use either?

@lsl yup that came up in the context of discussing this. However I think updating the data used by wordpress/i18n is still a seperate thing and won't fix this issue. wordpress/i18n isn't reactive so it still won't cause a re-render when setLocaleData() is called.

@p-jackson
Copy link
Member Author

@p-jackson p-jackson force-pushed the fix/onboarding-language-switching branch from 6e961b1 to f688d0e Compare December 14, 2020 22:56
@p-jackson p-jackson marked this pull request as ready for review December 14, 2020 23:10
@p-jackson p-jackson requested a review from a team December 14, 2020 23:12
@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 Dec 14, 2020
@ramonjd
Copy link
Member

ramonjd commented Dec 15, 2020

Does what it says on the tin 👍

Checked the /new flow, switched langs at every step. Also switched to RTL langs. All good.

After site creation I tested the launch flow (from the editor) by switching my account to German

Screen Shot 2020-12-15 at 3 17 59 pm

There's an issu on the plans step where the copy in the table doesn't update (see above) however the copy surrounding the table should update

I noticed that the text in the table didn't update when I toggled languages, but toggling between RTL languages seemed to force the update? Also I hit /new/plans?flags=gutenboarding/language-picker directly and it also seemed to honour each language switch. Just interesting that's all :)

Dec-15-2020 15-25-00

If that all sounds normal to you then it LGTM ✅

@andrewserong
Copy link
Member

@p-jackson I had a go at invalidating the resolution cache for the plans details grid in: #48391 — I wasn't 100% sure of my approach, so branched from this PR instead of committing here directly. Do you think something like that might solve it? I'm not sure if the cache invalidation solves the issue or just works around a different existing issue that needs to be resolved 🤔

@p-jackson p-jackson force-pushed the fix/onboarding-language-switching branch from f688d0e to 1ab32c2 Compare December 16, 2020 04:34
@p-jackson
Copy link
Member Author

👍 thanks @ramonjd, yes that all sounds normal. I'll merge once I know what the story is with the tests

Copy link
Member

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Works nicely for me, factoring in the known issues we're working on separately! 😀

@p-jackson p-jackson merged commit f50ea71 into trunk Dec 16, 2020
@p-jackson p-jackson deleted the fix/onboarding-language-switching branch December 16, 2020 04:59
@matticbot matticbot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 16, 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 [Type] Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Onboarding: Not all copy is updated when user switches languages
5 participants