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

react-i18n: Use I18n instances in context #39624

Merged
merged 13 commits into from
May 28, 2020
Merged

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Feb 24, 2020

Let's wait on #41207

Use instances of @wordpress/i18n to manage reactive i18n: WordPress/gutenberg#20318

TO DO:

  • We have code that depends on getting the locale. We may need to restore it.

const { __, i18nLocale } = useI18n();

const allSuggestions = useDomainSuggestions( { searchOverride: siteTitle, locale: i18nLocale } );

Testing

  • Test with DEMO: Use wp-i18n instances #39627 (instructions there)
  • Test /new routes and make sure they work as expected.
  • Confirm that sites are created with the appropriate language from /new! Try different user locales and locales via URL + incognito.

@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Feb 24, 2020

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

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

name                 parsed_size           gzip_size
entry-gutenboarding       +116 B  (+0.0%)      +34 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.

@sirreal
Copy link
Member Author

sirreal commented Feb 24, 2020

I've requested review in order to get early feedback as WordPress/gutenberg#20318 seems likely to move ahead with minimal changes.

@@ -0,0 +1,157 @@
/* eslint-disable */
Copy link
Member Author

Choose a reason for hiding this comment

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

This is duplicated from the upstream PR. It would come from the package and not exist here.

@sirreal sirreal requested a review from a team February 24, 2020 16:21
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 24, 2020
@sirreal
Copy link
Member Author

sirreal commented Feb 25, 2020

I've updated this with changes here removing i18nLocale from the context which seems like a good refinement of the API.

Merged to #39627 for testing.

The typecheck-strict failure can be ignored for now.

@sirreal sirreal marked this pull request as ready for review May 22, 2020 11:47
@sirreal
Copy link
Member Author

sirreal commented May 22, 2020

I've updated this and #39627. We know have a version of @wordpress/i18n that provides instances, so they can be used with our react-i18n library.

I'd love to get some feedback on this now, in particular @jsnajdr 🙂

@sirreal sirreal force-pushed the update/react-i18n-instances branch from 3c979b8 to cb93751 Compare May 26, 2020 11:08
_nx: i18n._nx.bind( i18n ),
_x: i18n._x.bind( i18n ),
isRTL: i18n.isRTL.bind( i18n ),
i18nLocale,
Copy link
Member Author

Choose a reason for hiding this comment

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

This may be very WordPress.com specific. I had an implementation that doesn't provide i18nLocale, but this property is already used in Gutenboarding.

@sirreal sirreal force-pushed the update/react-i18n-instances branch from 5ccde36 to 60dc646 Compare May 27, 2020 16:05
@sirreal
Copy link
Member Author

sirreal commented May 27, 2020

Switched to use useMemo over memize.

Rebased.

#39627 is also updated.

sirreal added a commit that referenced this pull request May 27, 2020
commit 60dc646
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Wed May 27 18:04:53 2020 +0200

    Add declarationMap

commit b388052
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Wed May 27 18:03:21 2020 +0200

    Remove memize dep

commit df6b9b2
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Wed May 27 17:56:32 2020 +0200

    Use useMemo over memize

commit 68b41e8
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Wed May 27 17:50:55 2020 +0200

    Add `i18nLocale` to README

commit 78df4e5
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Tue May 26 13:08:29 2020 +0200

    Use localeData over locale

commit c4ac3a3
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Fri May 22 13:41:08 2020 +0200

    Revert tsconfig change

commit 9a68361
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Fri May 22 13:30:09 2020 +0200

    Use i18nLocale property name

commit a5c49e3
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Fri May 22 12:44:26 2020 +0200

    Add localeSlug from i18n data

commit 2c16fe6
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Thu May 21 17:09:36 2020 +0200

    Add isRTL

commit 2695e21
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Thu May 21 17:07:32 2020 +0200

    Replace with packaged i18n

commit 548d17e
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Wed Feb 26 14:13:22 2020 +0100

    Add README

commit 2622d26
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Mon Feb 24 12:28:13 2020 +0100

    Use i18n instances

commit 476039f
Author: Jon Surrell <jon.surrell@automattic.com>
Date:   Mon Apr 20 15:12:48 2020 +0200

    Upgrade @wordpress/i18n
Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

Looks good, thanks so much for getting this implemented! 👍

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 28, 2020
@sirreal sirreal merged commit a01cadb into master May 28, 2020
@sirreal sirreal deleted the update/react-i18n-instances branch May 28, 2020 11:30
@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 May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants