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

Use LocaleProvider in calypso #47613

Merged
merged 9 commits into from
Dec 2, 2020
Merged

Use LocaleProvider in calypso #47613

merged 9 commits into from
Dec 2, 2020

Conversation

lsl
Copy link
Contributor

@lsl lsl commented Nov 20, 2020

Changes proposed in this Pull Request

  • Implements usage of LocaleProvider, useLocalizeUrl
  • Implements withLocalizeUrl
  • Makes use of both with/useLocalizeUrl
  • adds i18n-utils to domain-picker package

Testing instructions

Fixes #45610 🎉

@lsl lsl added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 20, 2020
@lsl lsl requested review from p-jackson and yuliyan November 20, 2020 06:38
@lsl lsl self-assigned this Nov 20, 2020
@matticbot
Copy link
Contributor

@lsl lsl changed the title Apply LocaleProvider to calypso Make use LocaleProvider in calypso, use localizeUrl in domain-picker Nov 20, 2020
This was referenced Nov 20, 2020
@sarayourfriend sarayourfriend changed the base branch from master to trunk November 20, 2020 16:09
Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

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

domain-picker is also used in the ETK's launch flow it will need a <LocaleProvider> added somewhere near the root. Probably in attach-launch-sidebar.tsx

@p-jackson
Copy link
Member

Lots of CI errors, rebased in case it needed it after the trunk branch rename.

@p-jackson
Copy link
Member

I'm at a bit of a loss as to what the build error's about. I see the error in the local build logs too, but I just can't see what the problem is.

@lsl
Copy link
Contributor Author

lsl commented Nov 24, 2020

Lots of CI errors, rebased in case it needed it after the trunk branch rename.

I don't think this took, tried a fresh rebase. Should probably retest, the initial testing was done against master that was probably a few days out of date.

@lsl
Copy link
Contributor Author

lsl commented Nov 24, 2020

That didn't work, testing moving to fresh PR in case its something build related #47692

@p-jackson
Copy link
Member

Maybe it's something to do with the filename changing from .ts to .tsx?

@p-jackson
Copy link
Member

p-jackson commented Nov 24, 2020

Actually I wonder if this is something to do build order.
If I do yarn clean && yarn to clean the dist folders and build the packages then I get a build error. Then if I follow up with another yarn then the build succeeds. The domain-picker package is complaining about i18n-utils not existing. But after starting the second build the i18n-utils/dist/types folder exists and now. Maybe the order packages are built is alphabetical so lerna builds domain-picker first, followed by i18n-utils. This might just be the first time that this has hit us.

Looking up lerna docs to see if there's a way to change the order workspace operations are run in.

@p-jackson
Copy link
Member

@scinos we had a broken build here before the domain-picker package was being built before i18n-utils, even though i18n-utils depends on domain-picker. Pretty unfortunate if we need to main this build ordering manually :(

From this comment it sounds like it's possible to have tsc build packages in the correct order if you also maintain a references list in each of the sibling package's tsconfig.json too. Which would be a lot of duplication.

TypeScript has a giant tracking issue to support this without having to maintain another list of dependencies.

@matticbot
Copy link
Contributor

matticbot commented Nov 24, 2020

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

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

name                   parsed_size           gzip_size
entry-main                 +1307 B  (+0.1%)      +91 B  (+0.0%)
entry-gutenboarding        +1173 B  (+0.1%)     +211 B  (+0.0%)
entry-login                +1103 B  (+0.1%)     +147 B  (+0.1%)
entry-domains-landing      +1103 B  (+0.2%)     +182 B  (+0.1%)

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

Sections (~10 bytes added 📈 [gzipped])

name     parsed_size           gzip_size
privacy        +14 B  (+0.0%)      +10 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

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

name                                           parsed_size           gzip_size
async-load-calypso-blocks-editor-launch-modal        +70 B  (+0.0%)      +19 B  (+0.0%)

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.

@scinos
Copy link
Contributor

scinos commented Nov 24, 2020

Looks like adding the reference to i18n-utils in packages/domain-picker/tsconfig.json fixed it (in #47692)

I agree it is not great to have to declare the dependencies in package.json and tsconfig.json, but I don't see how we can improve it until microsoft/TypeScript#25376 is resolved.

Maybe we can start by having some tsconfig.json linting that ensures all links are there? At least we would be able to produce a clear error, not something obscure that takes time to diagnose.

@p-jackson
Copy link
Member

Updated so that the locale slug in context updates when when i18n-calypso fires the 'change' event. Also fixed lint error and force pushed to bring things up to date.

@p-jackson
Copy link
Member

Looks like adding the reference to i18n-utils in packages/domain-picker/tsconfig.json fixed it

Awesome! I've brought that fix into this PR and closed #47692 since it was more just a trial PR to see if we were dealing with some GH or CI issue.

Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

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

Tested and everything to do with the change is working as expected.

I tested the domain picker inside the launch flow too (so inside the ETK). The HSTS tooltip doesn't appear when you click the button. It's a bug that needs to be fixed, but not i18n related.

The woo e2e tests are failing, but I think it's because there's currently something wrong with the e2e test user's site. When I run it locally the user's dashboard looks like this:
Screenshot 2020-11-30 at 5 14 43 PM

Maybe the e2e tests have just caught the site at a bad time 🤷

@p-jackson
Copy link
Member

Re-ran tests, passing now. Whatever was afflicting the woo test user must have been fixed.

@lsl lsl force-pushed the update/use-locale branch from 09cb5d6 to bdefa6b Compare December 2, 2020 03:52
@lsl
Copy link
Contributor Author

lsl commented Dec 2, 2020

Rebased, retested, everything seems ok.

@lsl lsl changed the title Make use LocaleProvider in calypso, use localizeUrl in domain-picker Use LocaleProvider in calypso Dec 2, 2020
@lsl lsl merged commit e9967ed into trunk Dec 2, 2020
@lsl lsl deleted the update/use-locale branch December 2, 2020 04:32
@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 Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Localize Support URL for HSTS Tooltip in Domain Picker
4 participants