Extract Gutenboarding language picker into @automattic/language-picker #46020

sarayourfriend · 2020-09-29T20:51:33Z

Changes proposed in this Pull Request

Extract the Gutenboarding language picker based on @wordpress/components into @automattic/language-picker allowing us to use it both in Gutenboarding and in Calypso.
Replaces the content of the language picker in Gutenboarding with the extracted version.

Testing instructions

Open /new/language-modal and confirm that it matches visually and behaviorally with production.

matticbot · 2020-09-29T20:51:35Z

Test live: https://calypso.live/?branch=try/extract-language-picker

simison · 2020-09-30T12:03:46Z

Using this inside a modal from @wordpress/components might be interesting step as well BTW.

sarayourfriend · 2020-09-30T17:32:30Z

@simison I love that idea! When we eventually implement the resulting component into the Calypso modal we can try to swap the Dialog for Modal

matticbot · 2020-10-08T23:41:47Z

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

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

name                 parsed_size           gzip_size
entry-gutenboarding       +656 B  (+0.0%)       +3 B  (+0.0%)

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

Sections (~80 bytes added 📈 [gzipped])

name         parsed_size           gzip_size
woocommerce       +130 B  (+0.0%)      +28 B  (+0.0%)
settings          +130 B  (+0.0%)      +26 B  (+0.0%)
account           +130 B  (+0.0%)      +26 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 (~115 bytes added 📈 [gzipped])

name                                parsed_size           gzip_size
async-load-quick-language-switcher       +130 B  (+0.2%)      +26 B  (+0.1%)
async-load-design-playground             +130 B  (+0.0%)      +26 B  (+0.0%)
async-load-design-blocks                 +130 B  (+0.0%)      +37 B  (+0.0%)
async-load-design                        +130 B  (+0.0%)      +26 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.

roo2

This looks good and I tested it with /new?flags=gutenboarding/language-picker turned on and it works well.

I just couldn't get the storybook working, can you please re-test that?

Also, did you consider adding this to packages/components rather than its own package? I think either way would be ok so no pressure to change it.

roo2 · 2020-10-12T01:23:17Z

package.json

@@ -154,6 +154,7 @@
 		"whybundled:server": "whybundled client/stats-server.json",
 		"composite-checkout-demo": "webpack-dev-server --config ./packages/composite-checkout/webpack.config.demo.js --mode development",
 		"components:storybook:start": "start-storybook -c packages/components/.storybook",
+		"language-picker:storybook:start": "start-storybook -c packages/language-picker/.storybook",


when I run this, I get an error on the storybook page in the browser "Exports is read only"

Same here. Could it be this: https://github.com/Automattic/wp-calypso/pull/46020/files#diff-dc7002f09ca033f4fc7a0c511508fd79bbdd671cd4fb904d9c60beb662f4c4c8R52

I'm not sure why this suddenly isn't working 🤔 Reading the error below:

./packages/language-picker/node_modules/@automattic/react-i18n/node_modules/@wordpress/is-shallow-equal/lib/objects.js/<@http://localhost:50609/vendors~main.74bd29d19c2e02c9b421.bundle.js:213500:1

It appears to be an issue within a dependency. The storybook worked before so I'm wondering if at some point I changed something while working on it that caused it to break (like updating the dependencies 🤔)

FWIW it works well for me now.

tyxla

This is exciting, thank you for the great work @saramarcondes!

I've left some comments, but this is a really great start! Let me know what you think.

packages/language-picker/.eslintrc.js

packages/language-picker/tsconfig.json

packages/language-picker/README.md

tyxla · 2020-10-13T12:58:26Z

package.json

@@ -154,6 +154,7 @@
 		"whybundled:server": "whybundled client/stats-server.json",
 		"composite-checkout-demo": "webpack-dev-server --config ./packages/composite-checkout/webpack.config.demo.js --mode development",
 		"components:storybook:start": "start-storybook -c packages/components/.storybook",
+		"language-picker:storybook:start": "start-storybook -c packages/language-picker/.storybook",


Same here. Could it be this: https://github.com/Automattic/wp-calypso/pull/46020/files#diff-dc7002f09ca033f4fc7a0c511508fd79bbdd671cd4fb904d9c60beb662f4c4c8R52

packages/language-picker/README.md

packages/language-picker/package.json

tyxla · 2020-10-13T13:00:49Z

packages/language-picker/src/index.ts

@@ -0,0 +1,2 @@
+export * from './language-picker';
+export { default } from './language-picker';


Not sure what we're trying to do here, could you please elaborate?

export * from '...' does not export the default, so if you have both named and default exports you need two separate export statements (unless there's another way that I'm not aware of). In this case we need to export the default (the component) and the named exports (the Language and LanguageGroup types)

tyxla · 2020-10-13T13:08:50Z

packages/language-picker/src/language-picker.tsx

+
+	return (
+		<>
+			<div className="language-picker__regions-label">{ __( 'regions' ) }</div>


I see we're using @wordpress/i18n here. This relies on the locale data to be already set. AFAIK, CalypsoI18nProvider currently does that for both Calypso-style localizations and Gutenberg-style localizations. But using that in a package assumes that this is somehow handled already in the consumer of the package. What are your thoughts about this?

also cc @automattic/i18n about it

If there's no provider, @automattic/react-i18n will use @worpdress/i18n instance with empty locale data, and therefore string won't get translated.
A good proposal for @automattic/react-i18n might be to fall back to the default @wordpress/i18n instance instead of creating an empty one, but that's a different subject.

Indeed, I've also explained more about that problem in #46020 (comment)

I've given this a bit more thought, and I think going with this makes sense. After all, it's not a component package's responsibility to take care of managing locale data itself. It should be the responsibility of the component consumer, or application.

So, @saramarcondes, I think going with @wordpress/i18n makes sense here, and is an acceptable trade-off. It aligns with our long-term goal, which is to use @wordpress packages for everything.

I have a small suggestion here. It might end up to be confusing for the user to see that we're using both @wordpress/i18n and @automattic/react-i18n, so it might be a better idea to always go with @automattic/react-i18n, and use either the useI18n hook or the withI18n high-order component. Both of them internally use @wordpress/i18n, so the idea here is to be consistent, while still keeping the opportunity to add additional package features and improvements to @automattic/react-i18n and benefit from them instantly. Let me know what you think.

I'd recommend one final thing: documenting in the package README that this package makes use of @wordpress/i18n and relies on the fact that the consumer of the package (the app in this case) is already providing the locale data. For Calypso, this is happening in the <CalypsoI18nProvider />, which does invoke setLocaleData() with the locale data coming from i18n-calypso.

Thanks again!

I'll update the README as you suggested.

One thing though, the package no longer uses react-i18n, only wordpress/i18n. Do you feel that it should be using react-i18n? (Admittedly I only changed it because using react-i18n was breaking the storybook in a mysterious way)

I wouldn't say it's a hard requirement as to which one to use. react-i18n is handy, because it adds some syntactic features on top of @wordpress/i18n, but since it is generally a wrapper around @wordpress/i18n, it doesn't really matter. It basically boils down to:

Use react-i18n if you need to use the HoC and/or the React hook.

Use @wordpress/i18n if you don't need to use any of the above.

Theoretically, it can turn out to be a blocker if react-i18n uses an older or newer version of @wordpress/i18n, but this shouldn't be a difficult problem to solve, as we have full control over reach-i18n, especially if it happens in Calypso.

tyxla · 2020-10-13T13:09:33Z

packages/language-picker/src/language-picker.tsx

+
+export type LanguageGroup = {
+	id: string;
+	name: ( translate: I18n[ '__' ] ) => string;


Not sure why we're renaming __() to __translate() here. Perhaps we should be explicit that we're using @wordpress/i18n?

This isn't renaming, it's the type annotation. i18n-calypso ultimately uses @wordpress/i18n does it not? translate is basically the same as the I18n function __, right?

Forgive me if I've been unclear in my comment, but this is indeed renaming, even if it happens under the hood.

i18n-calypso actually does not use @wordpress/i18n. It is a custom library that was built before @wordpress/i18n and uses a different mechanism for handling localization and translations. This is separate from the fact that the CalypsoI18nProvider provides locale data for both @wordpress/i18n and i18n-calypso.

So actually, __() is the @wordpress/i18n, but translate() is the i18n-calypso function. Both are localization functions, and for simple translations they work the same way, but they still have a lot of functional differences. For example translate() supports some additional features (for example arguments, components, context), while for @wordpress/i18n you need to use different functions sprintf() or _x() for example.

And by expecting __() but naming the argument translate, we're essentially confusing the package user as to what kind of argument to provide and how it's being used. It's also possible to introduce bugs because of that behavior.

Finally, I believe that the actual translation mechanism shouldn't be handled (or enforced) by the package. I've talked about this a bit more in https://github.com/Automattic/wp-calypso/pull/46020/files#r504487702.

Oh gosh, I misremembered. The original language picker uses @automattic/react-i18n which does use @wordpress/i18n, which was why I chose that type.

I understand the issue now is that we have two separate functions, translate and __. The solution in your other comment makes perfect sense to me. Thank you for your patience!

I agree the name is a bit misleading to use translate or even __ given the different contexts i18n-calypso, @wordpress/i18n and @automattic/react-i18n. It may be best to name it something generic like translationFunction. What we're describing is the shape of the name function in the LanguageGroup type, but folks will be free to name it whatever they want:

const lg: LanguageGroup = { // The name of the translation function passed to my name function // V name( translateMyName ) { return translateMyName("name"); } };

Regarding the type, it's probably more appropriate to use I18nReact["__"] from @automattic/react-i18n as the type of the function because that's ultimately where the function will come from. It's passing the @wordpress/i18n __ type through so it's not too important. @wordpress/i18n __ is unfortunately generic right now (Function type) but something that can be improved Gutenberg-side.

@wordpress/i18n __ is unfortunately generic right now (Function type) but something that can be improved Gutenberg-side.

PR to improve function types: WordPress/gutenberg#26171

tyxla · 2020-10-13T13:15:35Z

packages/language-picker/src/language-picker.tsx

+
+export type LanguageGroup = {
+	id: string;
+	name: ( translate: I18n[ '__' ] ) => string;


It feels a bit off to pass translate as an argument here. Did we mean to use it directly in the code? A function that returns a string should be enough IMHO.

The language groups are defined in a static file, not inside any specific context. I'm not sure how we could have translated names without passing translate to the static file unless we changed the way language groups are defined (as in, defining them in a specific runtime context rather than just a generic module). What are your thoughts for the alternatives to make this work with translated names still?

I'd expect that we would just use a function that returns a string. See https://github.com/Automattic/wp-calypso/blob/try/extract-language-picker/client/lib/plans/plans-list.js#L25 for example.

By going with this approach, we:

Ensure that localization functions will be invoked as late as possible, which is necessary to be sure we've loaded all the locale data already.

Avoid the necessity to pass a specific localization function as an argument.

Stay flexible and not enforce a specific i18n framework. By expecting just a function that returns a string, we let the package consumer decide what i18n tools to use, vs forcing them to use a particular one.

Let me know if that makes sense.

Yes, thank you for the explanation and example!

@tyxla Does this also apply to localization that happens within the component itself (as in the case of the Regions label?) Would we just expect the consumer to dependency inject an i18n provider that had the signature string => string? Or is it safe to import __ directly from @wordpress/i18n as long as it is invoked as late as possible, as you said (like during rendering)?

is it safe to import __ directly from @wordpress/i18n as long as it is invoked as late as possible, as you said (like during rendering)?

I'd say it's safe to do that. That's why we wrap them in functions, so invoking them late is a good warranty that it'll be done after the locale data has already loaded.

Note: I'm not saying it's a perfect solution. We've been limited in Calypso by that for a while now, too, and we're likely to stay with that limitation in the future.

That being said, I think it's an acceptable trade-off.

sarayourfriend · 2020-10-13T16:43:18Z

@roo2 The reason I didn't put it into @automattic/components (I've updated the title) is because moving it in there was causing it to get imported on app-start (because those components are always used) and because this depends on the non-SSR compatible @wordpress/components it would cause the app to crash when starting. Having it in a separate package is a temporary workaround for that issue. Once the @wordpress scope is updated this PR can be rebased and then I could move it into the components (or we can merge this PR and then move it into components in a subsequent PR). The goal is that it would live in components though.

tyxla · 2020-10-14T08:46:43Z

packages/language-picker/src/language-picker.stories.tsx

+	},
+];
+
+export const _default = () => {


How is this _default export expected to be used?

This creates a "Default" example in Storybook. This is the same pattern followed by Gutenberg's usage of Storybook:

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/form-toggle/stories/index.js#L26

scinos · 2020-10-14T14:54:33Z

packages/language-picker/package.json

+	],
+	"types": "dist/types",
+	"dependencies": {
+		"@automattic/react-i18n": "file:../react-i18n",


We should import using the version

Suggested change

"@automattic/react-i18n": "file:../react-i18n",

"@automattic/react-i18n": "^1.0.0-alpha.0",

tyxla · 2020-10-15T10:25:20Z

packages/language-picker/src/language-picker.tsx

+	onSelectLanguage: ( language: Language ) => void;
+	languages: Language[];
+	languageGroups: LanguageGroup[];
+	defaultLananguageGroupId: string;


There's a typo here: defaultLananguageGroupId -> defaultLanguageGroupId.

Suggested change

defaultLananguageGroupId: string;

defaultLanguageGroupId: string;

tyxla · 2020-10-15T10:35:41Z

packages/language-picker/src/language-picker.tsx

+	onSelectLanguage,
+	languages,
+	languageGroups,
+	defaultLananguageGroupId,


Suggested change

defaultLananguageGroupId,

defaultLanguageGroupId,

tyxla · 2020-10-15T10:35:47Z

packages/language-picker/src/language-picker.tsx

+	languageGroups,
+	defaultLananguageGroupId,
+}: Props ) => {
+	const [ filter, setFilter ] = useState( defaultLananguageGroupId );


Suggested change

const [ filter, setFilter ] = useState( defaultLananguageGroupId );

const [ filter, setFilter ] = useState( defaultLanguageGroupId );

tyxla · 2020-10-15T10:36:04Z

client/landing/gutenboarding/onboarding-block/language/index.tsx

-					</div>
+					<LanguagePicker
+						languageGroups={ LANGUAGE_GROUPS }
+						defaultLananguageGroupId={ DEFAULT_LANGUAGE_GROUP }


Suggested change

defaultLananguageGroupId={ DEFAULT_LANGUAGE_GROUP }

defaultLanguageGroupId={ DEFAULT_LANGUAGE_GROUP }

tyxla · 2020-10-15T10:36:19Z

packages/language-picker/src/language-picker.stories.tsx

+			onSelectLanguage={ ( language ) => console.log( language ) }
+			languageGroups={ LANGUAGE_GROUPS }
+			languages={ LANGUAGES }
+			defaultLananguageGroupId="popular"


Suggested change

defaultLananguageGroupId="popular"

defaultLanguageGroupId="popular"

tyxla · 2020-10-15T10:39:40Z

packages/language-picker/src/language-picker.tsx

+	onSelectLanguage,
+	languages,
+	languageGroups,
+	defaultLananguageGroupId,


Additionally, I wonder if we can't always just default to the first item in languageGroups. That's not only one prop less, it also complies with the WordPress philosophy: decisions, not options.

tyxla · 2020-10-15T10:46:45Z

packages/language-picker/src/language-picker.tsx

+
+	return (
+		<>
+			<div className="language-picker__regions-label">{ __( 'regions' ) }</div>


I've given this a bit more thought, and I think going with this makes sense. After all, it's not a component package's responsibility to take care of managing locale data itself. It should be the responsibility of the component consumer, or application.

So, @saramarcondes, I think going with @wordpress/i18n makes sense here, and is an acceptable trade-off. It aligns with our long-term goal, which is to use @wordpress packages for everything.

I have a small suggestion here. It might end up to be confusing for the user to see that we're using both @wordpress/i18n and @automattic/react-i18n, so it might be a better idea to always go with @automattic/react-i18n, and use either the useI18n hook or the withI18n high-order component. Both of them internally use @wordpress/i18n, so the idea here is to be consistent, while still keeping the opportunity to add additional package features and improvements to @automattic/react-i18n and benefit from them instantly. Let me know what you think.

I'd recommend one final thing: documenting in the package README that this package makes use of @wordpress/i18n and relies on the fact that the consumer of the package (the app in this case) is already providing the locale data. For Calypso, this is happening in the <CalypsoI18nProvider />, which does invoke setLocaleData() with the locale data coming from i18n-calypso.

tyxla

This is looking great overall!

There's one blocker here: the language groups no longer work, because they expect us to pass a translate function as an argument to the language group name function, but we no longer do that.

tyxla · 2020-10-20T11:41:05Z

packages/language-picker/src/style.scss

+@import '~@wordpress/base-styles/mixins';
+@import '~@wordpress/base-styles/variables';
+
+.language-picker__language-group {


Same as #46424 - do you think it makes sense to have a common wrapping class for all styles in this package, to make sure all styles are properly encapsulated and have a smaller chance to bleed to outside elements?

tyxla · 2020-10-20T11:41:46Z

packages/language-picker/src/style.scss

+	width: 100%;
+}
+
+.language-picker__language-group > span {


Nit: I wonder if this span can have its own className that will always be there; so we could target it by class name and not by element name.

tyxla · 2020-10-20T11:56:20Z

packages/language-picker/src/language-picker.tsx

+
+export type LanguageGroup = {
+	id: string;
+	name: () => string;


Unfortunately, we're going to hit into an issue with this now:

Because the existing LANGUAGE_GROUPS constant expects that their name will get a translation function as an argument, and we're no longer passing it here, it's going to fail.

We should either:

Address this in the constant definition, by using __() there directly, vs expecting a translation function. Can you think of any downside to this approach, @Automattic/i18n @yuliyan?

Create a wrapper constant that uses LANGUAGE_GROUPS but predefines the name by passing __() as an argument to the function.

I personally prefer the first approach - ideally, it shouldn't be too hard to replace the instances of languageGroup.name( translate ) to languageGroup.name( __` ) - I expect those to be only in the language picker component anyway.

That makes sense (I thought I had fixed this but I guess I got the Storybook example and the live constants confused 😖)

Something I'm still confused about is whether we need to include the text domain global variable as described by @alshakero in the recent P2 here: pbAok1-1Ao-p2

If we do need to include the __i18n_text_domain__ variable in the component (both here and in search) then do we need to include it in these constants as well?

@tyxla After making this change actually there is a new but subtle bug. When selecting English from the langauge picker in Gutenboarding (accessed via /new?flags=gutenboarding/language-picker) it does not change the region languages to English (whereas for other languages it does work and in production it indeed will change to English when English is selected).

To reproduce:

Go to /new?flags=gutenboarding/language-picker on this branch

Click the language picker from the upper right hand corner

Select a languag other than English. Reopen the language picker and note that the regions are now listed in the langauge you selected.

Now select English. Reopen the language picker and note that the regions are still listed in the previous language you selected.

Repeat this several times going between languages that are not English and note that the regions change every time to the newly selected language except for when you choose English.

In production selecting English will behave the same as the rest of the languages.

Something I'm still confused about is whether we need to include the text domain global variable as described by alshakero in the recent P2 here: pbAok1-1Ao-p2

@Automattic/ganon are exploring these questions currently.

@Automattic/ganon are exploring these questions currently.

Latest WIP from @p-jackson here #46557

sarayourfriend · 2020-10-23T18:51:38Z

@tyxla I've pushed an update to this branch that fixes the translation issues I outlined previously. It makes some rather big changes that I'm not super comfortable with. I'd love your input. I was not able to get it working the way we had it before. The way that we're doing things now will require us to change the Calypso implementation to use react-i18n but I think that's okay given we want to move in the direction of using @wordpress packages anyway.

I also removed storybook from this package because react-i18n is incompatible with it. The story for the component also had very limited usefulness I think, with difficulty we could have made it more useful but not without making some big changes to how we manage and import languages.

In any case. I'm hoping that my changes regarding translations make sense. See if you can find a different way to address the bug if you'd like, or if you can find the root cause for English not getting activated for the component's __.

Feedback addressed

tyxla · 2020-10-27T12:24:53Z

@tyxla I've pushed an update to this branch that fixes the translation issues I outlined previously. It makes some rather big changes that I'm not super comfortable with. I'd love your input. I was not able to get it working the way we had it before. The way that we're doing things now will require us to change the Calypso implementation to use react-i18n but I think that's okay given we want to move in the direction of using @wordpress packages anyway.

I also removed storybook from this package because react-i18n is incompatible with it. The story for the component also had very limited usefulness I think, with difficulty, we could have made it more useful but not without making some big changes to how we manage and import languages.

In any case. I'm hoping that my changes regarding translations make sense. See if you can find a different way to address the bug if you'd like, or if you can find the root cause for English not getting activated for the component's __.

I've taken a look at the approach and concluded there isn't an easy way around it right now. If we fall back to using __() from @wordpress/i18n, in this instance we'll end up in a case where the components that use it aren't subscribed to changes in the locale data. Even if we force a re-render, the @wordpress/i18n instance will use the old locale data. This is why we have react-i18n in the first place, after all. But the approach there works only for components, and not for static functions that aren't React components.

So to work around it, we should pass a __() from react-i18n to the constants. That's what you did essentially, and after weighing the trade-offs, I think it's one that we can live with. I actually like the approach, because it makes it more clear why we're doing it, and somewhat isolates the root of the problem.

That being said, feel free to add some documentation in the code to make sure that what we're doing is clear to our future selves.

I've dismissed my previous review, and from my perspective, this is good to go. Ideally, I'd like to get a ✅ from @Automattic/ganon before merging.

Nice work and thanks for the hard work here 👏

jsnajdr · 2020-10-27T12:31:44Z

The way that we're doing things now will require us to change the Calypso implementation to use react-i18n but I think that's okay given we want to move in the direction of using @wordpress packages anyway.

Using the react-i18n package is fine and even desired. The plan has always been to develop it in Calypso and then contribute back to @wordpress after it's battle-tested.

The react-i18n approach is more flexible and more in line with React philosophy than the global singletons and global __() functions in the base @wordpress/i18n package.

tyxla · 2020-10-27T12:40:14Z

The way that we're doing things now will require us to change the Calypso implementation to use react-i18n but I think that's okay given we want to move in the direction of using @wordpress packages anyway.

Using the react-i18n package is fine and even desired. The plan has always been to develop it in Calypso and then contribute back to @wordpress after it's battle-tested.

The react-i18n approach is more flexible and more in line with React philosophy than the global singletons and global __() functions in the base @wordpress/i18n package.

I was talking about a different trade-off here (passing __() down vs being able to directly use it), but just to confirm that I completely agree with what Jarda said.

ramonjd · 2020-10-28T02:32:57Z

I've dismissed my previous review, and from my perspective, this is good to go. Ideally, I'd like to get a ✅ from @Automattic/ganon before merging.

I don't have anything to add. Maybe @andrewserong @p-jackson or @roo2 have.

That said, we'll be testing integration as part of the i18n Gutenboarding effort, so we'll report anything we see then.

Thanks to everyone for their work on this!

p-jackson

Tested and worked as expected. LGTM!

The __i18n_text_domain__ constant is only really needed if we intend to include @automattic/language-picker in a WordPress plugin, e.g. the Editing Toolkit. Otherwise leaving it out is fine and it'll default to using the default text domain. This is what Calypso and Gutenboarding use, so it'll work fine.
You've probably seen it, but __i18n_text_domain__ is defined here for Calypso and here for the Editing Toolkit. So this package is usable in either of those contexts now. If you try and install this package in a context that doesn't define __i18n_text_domain__ then this package will crash. But that's fine for now right?

I left a couple of nit picks but they're optional really, I think this is good to go as is, we can iterate on i18n stuff as we go IMO.

p-jackson · 2020-10-28T03:13:56Z

packages/language-picker/src/language-picker.tsx

+	languageGroups: LanguageGroup[];
+};
+
+const LanguagePicker = ( { onSelectLanguage, languages, languageGroups }: Props ) => {


Optional: you can fix the eslint warning about defining module boundaries by using the React.FC type.

Suggested change

const LanguagePicker = ( { onSelectLanguage, languages, languageGroups }: Props ) => {

const LanguagePicker: React.FC< Props > = ( { onSelectLanguage, languages, languageGroups } ) => {

p-jackson · 2020-10-28T03:19:55Z

packages/language-picker/src/constants.ts

+import { LanguageGroup } from './language-picker';
+import { I18nReact } from '@automattic/react-i18n';


Minor nit pick: using import type makes it easier for tooling to optimise the case where a package is only geing imported for a type.

Suggested change

import { LanguageGroup } from './language-picker';

import { I18nReact } from '@automattic/react-i18n';

import type { LanguageGroup } from './language-picker';

import type { I18nReact } from '@automattic/react-i18n';

Not a big deal, it just stood out to me because I thought we had an eslint warning about this. Eslint appears to be working in this new package though so I must be mistaken.

tyxla · 2020-10-28T09:08:25Z

Thanks to everyone for their work on this!

Indeed, high five! Great team effort, folks 🙌

sarayourfriend · 2020-10-28T17:08:55Z

Thanks for the feedback @p-jackson. I'll have another follow-up PR to implement this language picker into Calypso and will address the issues you've pointed out there, if that's okay with you! Otherwise I can open a fast-follow PR to specifically address these if you think they should be taken care of sooner.

p-jackson · 2020-10-29T02:38:45Z

I can open a fast-follow PR to specifically address these if you think they should be taken care of sooner.

@saramarcondes I don't think there's a need, they're the sort of tidy ups that can go whenever

sarayourfriend force-pushed the try/extract-language-picker branch 2 times, most recently from 7b34ee8 to 301004e Compare October 8, 2020 21:25

sarayourfriend marked this pull request as ready for review October 8, 2020 21:26

sarayourfriend requested review from a team October 8, 2020 21:26

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 8, 2020

sarayourfriend requested a review from tyxla October 8, 2020 21:26

sarayourfriend self-assigned this Oct 8, 2020

sarayourfriend added [Feature Group] Signup & Site Onboarding Tools for user registration and onboarding new users to the site. Components labels Oct 8, 2020

sarayourfriend mentioned this pull request Oct 9, 2020

Use LanguagePicker in Calypso #46328

Closed

roo2 reviewed Oct 12, 2020

View reviewed changes

tyxla reviewed Oct 13, 2020

View reviewed changes

sarayourfriend changed the title ~~Extract Gutenboarding language picker into @automattic/components~~ Extract Gutenboarding language picker into @automattic/language-picker Oct 13, 2020

tyxla reviewed Oct 14, 2020

View reviewed changes

scinos reviewed Oct 14, 2020

View reviewed changes

ramonjd mentioned this pull request Oct 15, 2020

Gutenboarding: i18n #37665

Closed

46 tasks

tyxla reviewed Oct 15, 2020

View reviewed changes

sarayourfriend mentioned this pull request Oct 15, 2020

Extract the search component into @automattic/search #46424

Merged

sarayourfriend requested a review from tyxla October 16, 2020 13:17

ramonjd added [Goal] New Onboarding previously called Gutenboarding [Pri] High Address as soon as possible after BLOCKER issues labels Oct 19, 2020

tyxla previously requested changes Oct 20, 2020

View reviewed changes

sarayourfriend requested review from tyxla and roo2 October 21, 2020 17:39

autumnfjeld mentioned this pull request Oct 22, 2020

Localize Support URL for HSTS Tooltip in Domain Picker #45610

Closed

sarayourfriend force-pushed the try/extract-language-picker branch from a0a1d3c to fd48015 Compare October 22, 2020 17:18

sarayourfriend added 11 commits October 23, 2020 10:46

Add LanguagePicker as package

ae0330d

Use LanguagePicker in Gutenboarding

560956a

Correct references to language-picker package

467acae

Clean up package

755018c

Remove react-i18n dependency that was breaking storybook

ab755b9

Fix typo and add i18n documentation

e632295

Commit missing yarn.lock changes

050e38f

Remove non-existent prop

29342ed

Use wp/i18n to translate region names

212a8b9

Wrap all styles and use more specific name

e1d2ca7

Fix various translation bugs

c181730

sarayourfriend force-pushed the try/extract-language-picker branch from fd48015 to c181730 Compare October 23, 2020 18:44

Remove language-picker storybook

bf3aebc

p-jackson approved these changes Oct 28, 2020

View reviewed changes

sarayourfriend merged commit 58b9a18 into master Oct 28, 2020

sarayourfriend deleted the try/extract-language-picker branch October 28, 2020 17:09

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 28, 2020

		@@ -0,0 +1,2 @@
		export * from './language-picker';
		export { default } from './language-picker';

	"@automattic/react-i18n": "file:../react-i18n",
	"@automattic/react-i18n": "^1.0.0-alpha.0",

	defaultLananguageGroupId: string;
	defaultLanguageGroupId: string;

	const [ filter, setFilter ] = useState( defaultLananguageGroupId );
	const [ filter, setFilter ] = useState( defaultLanguageGroupId );

	defaultLananguageGroupId={ DEFAULT_LANGUAGE_GROUP }
	defaultLanguageGroupId={ DEFAULT_LANGUAGE_GROUP }

	defaultLananguageGroupId="popular"
	defaultLanguageGroupId="popular"

	const LanguagePicker = ( { onSelectLanguage, languages, languageGroups }: Props ) => {
	const LanguagePicker: React.FC< Props > = ( { onSelectLanguage, languages, languageGroups } ) => {

		import { LanguageGroup } from './language-picker';
		import { I18nReact } from '@automattic/react-i18n';

Extract Gutenboarding language picker into @automattic/language-picker #46020

Extract Gutenboarding language picker into @automattic/language-picker #46020

Conversation

sarayourfriend commented Sep 29, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Sep 29, 2020

simison commented Sep 30, 2020

sarayourfriend commented Sep 30, 2020

matticbot commented Oct 8, 2020 • edited Loading

roo2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuliyan Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend commented Oct 13, 2020

Choose a reason for hiding this comment

sarayourfriend Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

scinos Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend commented Oct 23, 2020

tyxla commented Oct 27, 2020

jsnajdr commented Oct 27, 2020

tyxla commented Oct 27, 2020

ramonjd commented Oct 28, 2020

p-jackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla commented Oct 28, 2020

sarayourfriend commented Oct 28, 2020

p-jackson commented Oct 29, 2020

sarayourfriend commented Sep 29, 2020 •

edited

Loading

matticbot commented Oct 8, 2020 •

edited

Loading

sarayourfriend Oct 13, 2020 •

edited

Loading

sarayourfriend Oct 13, 2020 •

edited

Loading

yuliyan Oct 13, 2020 •

edited

Loading

sarayourfriend Oct 14, 2020 •

edited

Loading

sarayourfriend Oct 14, 2020 •

edited

Loading

scinos Oct 14, 2020 •

edited

Loading