-
Notifications
You must be signed in to change notification settings - Fork 2k
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 LanguagePicker in Calypso #46328
Conversation
{ shouldDisplayRegions ? ( | ||
<> | ||
<div className="language-picker__regions-label">{ __( 'regions' ) }</div> | ||
<div className="language-picker__languages-label">{ __( 'language' ) }</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 17 times:
translate( 'Language', { context: 'logged-out-homepage'} )
ES Score: 8
See 2 additional suggestions in the PR translation status page
<div className="language-picker__languages-label">{ __( 'language' ) }</div> | ||
</> | ||
) : ( | ||
<div className="language-picker__search-results-label">{ __( 'search result' ) }</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 67 times:
translate( 'Search results' )
ES Score: 6
See 2 additional suggestions in the PR translation status page
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~244 bytes removed 📉 [gzipped])
Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded. App Entrypoints (~27099 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~152483 bytes added 📈 [gzipped])
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 (~194138 bytes added 📈 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
a09e3ec
to
ace90f8
Compare
{ shouldDisplayRegions ? ( | ||
<> | ||
<div className="language-picker-component__regions-label">{ __( 'regions' ) }</div> | ||
<div className="language-picker-component__languages-label">{ __( 'language' ) }</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 17 times:
translate( 'Language', { context: 'logged-out-homepage'} )
ES Score: 8
See 2 additional suggestions in the PR translation status page
ace90f8
to
7c0541c
Compare
{ shouldDisplayRegions ? ( | ||
<> | ||
<div className="language-picker-component__regions-label">{ __( 'regions' ) }</div> | ||
<div className="language-picker-component__languages-label">{ __( 'languages' ) }</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 48 times:
translate( 'Languages', { context: 'Help topic'} )
ES Score: 10
}; | ||
|
||
const LanguagePickerModal = ( { | ||
languages, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
Was there any reason not to fetch the localized languages from the /i18n/language-names
endpoint (to enable localized searching)?
Granted, there is a bit of data mashing to get the filtered results, but we're removing a feature so I just wanted to make sure it's intentional.
We're using a query component for that at the moment <QueryLanguageNames />
in the deleted modal picker file.
Also asked here p9oQ9f-kT-p2#comment-548
Thanks!
cc @Automattic/i18n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ramonjd Do you have any pointers on how to do this from within Gutenboarding? We just need to get the response and the LanguagePicker component can already handle the result. I can see how to retrieve it in Calypso just using the existing query component, but that doesn't seem like it will work in Gutenboarding. I'm not at all familiar with how the data stores and all that stuff works.
Scratch that: You can see from my latest pushes that I've been able to retrieve the localized language names. However, they don't come back for the correct locale and I'm unsure how to fix this, I'm wondering if you could help with that instead?
Also, I accomplished this by adding a new i18n store to data-stores
. I'm not sure if this is the right way to go so open to suggestions for other places to put that new store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they don't come back for the correct locale
The endpoint doesn't check the user's default locale (assuming a user is logged in), but relies on the _locale
query parameter.
https://public-api.wordpress.com/wpcom/v2/i18n/language-names?_locale=es
Otherwise the "localized" names will be in English.
So, I guess we'd have to pass the current locale to the selector (and indirectly to the resolver).
Normally, the locale will be stored in the URL fragment, e.g., /new/es
, or /new/language-modal/de
I see that getLocale exists to check the URL for the existence of a valid locale.
You can test with the Language picker enabled by adding the following flag: /new/?flags=gutenboarding/language-picker
Having said all this, one of our tasks for i18n Gutenboarding was to enable the language picker, so if you get the data-store/i18n selector/resolver/apiFetch accepting a locale then we could look at passing the right one from Gutenboarding later if you run out of time.
adding a new i18n store to data-stores
👍 That's what I was going to suggest - nice work. It's where most of the state-related stuff for Gutenboarding lives. I'll still ping @razvanpapadopol and @yansern, who know the Gutenboarding architecture back to front to make sure. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _locale
query parameter should be added automatically to every REST request, based on what's currently in Redux state.ui.language
. There is a dedicated wpcom
interceptor handler for that. For the older WP.COM endpoints (as opposed to the newer WP REST ones), the parameter name is locale
, without the _
.
Gutenberg's apiFetch
code has a similar handler that, however, always adds _locale=user
, telling the server to use the logged-in user's locale rather than default en
. In logged-out mode, however, this does nothing.
The Core REST code seems to not support anything other than user
: https://github.com/WordPress/WordPress/blob/master/wp-includes/l10n.php#L143-L145 But an endpoint like i18n/language-names
can check the _locale
param on its own.
Normally, the locale will be stored in the URL fragment, e.g.,
/new/es
, or/new/language-modal/de
In most cases, the locale is determined from the current logged in user. But the locale from URL should make its way to the Redux and i18n-calypso
, too. The Signup and Login handlers that use these locale suffixes should have code for that.
Hopefully the above will be helpful when debugging the _locale
param. Calypso code should always add it, and Gutenberg code will probably need to add it explicitly.
return ( | ||
<Dialog | ||
isVisible | ||
onClose={ onClose } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're also removing an existing tracking event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This event was added to track whether search was being used to make a decision about whether to keep search. We decided to keep search independent of the event so I think the event can be safely removed now.
It's looking really nice! ❤️ I just have a few questions about dissimilarities between this and the current search functionality. I'll leave the design 👀 to @Automattic/dotcom-create-design
There are also a few missing translations, but I expect that they're new. |
This PR is also missing the empathy mode checkbox as well as the percent translated information. This is missing from the designs so I'm not sure where to put it. I've pinged design here p9oQ9f-kT-p2#comment-552 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this running, it's exciting to see it working!
I'd say this is great work overall! I've left some comments and questions for things I've found while testing.
Furthermore, I'd love it if on top of @Automattic/ganon, @Automattic/i18n could also take a look. I remember @yuliyan has been spending some time with the language picker.
languages={ languages } | ||
onClose={ this.handleClose } | ||
onSelected={ this.selectLanguage } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So selectedLanguage
won't be necessary anymore and can be removed?
languages={ languages } | ||
onClose={ this.handleClose } | ||
onSelected={ this.selectLanguage } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to no longer using this.selectedLanguage
here, is it concerning we remove the support for empathyMode
and useFallbackForIncompleteLanguages
that were also set here previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to re-incorporate those two checkboxes, I've pinged design (link above) to get information about how to handle those things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, looking forward to what comes out from that conversation.
<div className="language-picker-component__language-buttons"> | ||
{ getFilteredLanguages().map( ( language ) => ( | ||
{ languagesToRender.map( ( language ) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/> | ||
</div> | ||
<div className="language-picker-component__search-desktop"> | ||
<Search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW when searching for a language and pressing "enter / return", a form seems to get submitted in the background (props @yuliyan for noticing that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch, I forgot to stopPropagation
on the form event.
<FlexBlock className="language-picker-component__heading"> | ||
<Flex justify="space-between" align="left"> | ||
<FlexItem className="language-picker-component__title wp-brand-font"> | ||
{ headingTitle || __( 'Select a language', __i18n_text_domain__ ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some instances we use a textdomain, and in some we don't. What is the rationale behind that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be being used for all instances. Places that aren't are a mistake I believe (except for createLanguageGroups
where the passed in __
will have a text domain configured I believe... perhaps that is incorrect?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be being used for all instances.
Gotcha. That's aligned to pbAok1-1Ao-p2 so we should be good.
createLanguageGroups
where the passed in__
will have a text domain configured I believe
I think that's not the case, it doesn't have the textdomain configured, but it doesn't need to have a textdomain, because where we pass __
is used only in Calypso context, where we don't need to specify a textdomain. If the package is used externally, then we can just pass a bound __()
that specifies a textdomain, which isn't ideal, but will do the job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... In that case, we might as well pass __i18n_text_domain__
in createLanguageGroups
right? The signature should allow us to regardless and it doesn't cost anything right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might as well pass
__i18n_text_domain__
in createLanguageGroups
I'm assuming yes, but I'm not 100% sure what you mean, could you please demonstrate it with an example?
onChange={ ( { selectedItem } ) => selectedItem && setFilter( selectedItem.key ) } | ||
/> | ||
<div className="language-picker-component__search-mobile"> | ||
<Search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an odd popping when opening the search on mobile; you can see that the "Select a Language" title moves for a little before returning to its place. I haven't investigated what it is, but it seems like the search bar gets too wide for a second when opening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyxla Do you mind recording your screen with this happening? I can't reproduce it at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to do that, but for some reason I'm getting an error when trying to build with the latest branch:
@automattic/data-stores: src/auth/actions.ts(4,24): error TS7016: Could not find a declaration file for module '@wordpress/data-controls'
Are you able to reproduce that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the CI is failing on this as well. I can't reproduce it locally but I'll rebase against master and see if that changes anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed now that search is only 240px wide on desktop. Let me know if you can still reproduce it @tyxla, thanks! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't reproduce anymore, I think it's working well now. The only nit I have here is that while the search is opening, there's a scroll appearing. It disappears once the search box ends in its place. Is this something we can easily prevent?
464a217
to
6b715ab
Compare
@scinos Do you mind taking a look at the test failure? I believe it may have something to do with recent changes to module resolution in tests. The module works fine when running the app but fails to resolve the language picker when running tests. |
I think the unit test are failing because the changes in #47002 . After that change we don't produce A potential solution would be to change |
Yes, the unit tests are failing because the Jest resolver wants to use the Debugging As this is a too deep rabbit hole that I'm not willing to descend into right now, I simply partially reverted #47002 in d62a0fc and enabled We should try to remove the |
17eb778
to
69b5752
Compare
@ollierozdarz Hi there! Do you mind giving this PR a spin for a design review? It can be accessed here: https://calypso.live/?branch=add/use-lp-in-calypso Please follow the instructions in the PR description for how to access the relevant areas 🙂 |
@@ -19321,6 +19278,11 @@ lodash@*, lodash@^4.0.0, lodash@^4.1.1, lodash@^4.13.1, lodash@^4.15.0, lodash@^ | |||
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.15.tgz#b447f6670a0455bbfeedd11392eff330ea097548" | |||
integrity sha512-8xOcRHvCjnocdS5cpwXQXVzmmh5e5+saE2QGoeQmbKmRS6J3VQppPOIt0MnmE+4xlZoumy0GPG0D0MVIQbNA1A== | |||
|
|||
[email protected], lodash@^4.16.4, lodash@^4.17.20: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we can dedupe this one with the previous lodash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuliyan Fix is up! Thanks again! |
@saramarcondes, I tested in Firefox, and it seems like it has the same bug but it's less noticeable. From what I see, the container that holds the search bar overflows horizontally in both browsers, but the difference between Chrome and Firefox is that, Firefox keeps the scroll position of the container to the left and Chrome scrolls right to keep the overflown elements in view. Edit: Just saw the fix, looks good 👍 |
Hi @saramarcondes, I think this meets the functionally we have been chatting about. Good job. If it's not too late, I'd probably suggest putting a Grey border around the search input in it's unselected state for the desktop view, as I worry it's easy to miss. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your continuous great work on this one, @saramarcondes!
One thing that I noticed was that on mobile (480px), things looked misaligned a little bit:
Perhaps that impression is caused by several facts:
- The search button is a bit too wide, and it's not aligned to the right.
- The cancel button is not left aligned with anything because of the inner button padding
- Title seems to be more indented than the select box and the list below.
That's just my personal opinion, though, and it might be best to consult with a designer on this one.
Also, I've started reviewing the entirety of this PR today, but I'm afraid it's grown beyond the point where it should be a single PR already. Reviewing such high-impact PRs can be really difficult, and I personally can't be confident that I'm not missing a bunch of stuff when reviewing and testing it. Have you considered splitting it into multiple PRs? One way we could split it to multiple PRs could be:
- Introducing i18n functionality to the data-stores package
- Changes to the language-picker package
- Changes to the search package
- Updating language picker in Gutenboading
- Updating language picker in Calypso language picker component
Sorry for proposing this so late, but I'm starting to feel that I can miss something while reviewing, and I'm not reviewing it thoroughly and with the usual quality.
What do you think @saramarcondes?
I agree with @tyxla that splitting the PR could improve the review process. For me personally, there are changes in some packages and areas of the code that I'm unfamiliar with, and if those were separated it would've been easier to only get involved in PRs where I can provide more valuable input. However, given that the PR is already in late stage, I'm not sure how much effort would it take to split it and if would be worth it. I don't want to influence your decision in any way, just sharing my thoughts. |
Changes proposed in this Pull Request
Testing instructions
/new?flags=gutenboarding/language-picker
(click the language picker in the upper right-hand corner),/me/account
and/settings/general/:site
's language pickers match the spec from @olaolusoga and @ollierozdarz here: p9oQ9f-kT-p2