3770 multi language improvements#4134
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I've added a change that makes the legal text aligned to the left as suggested by figma. Here are some relevant screenshots: android ios |
|
Looks great! |
src/components/Icon/Expensicons.js
Outdated
| import Close from '../../../assets/images/close.svg'; | ||
| import CreditCard from '../../../assets/images/creditcard.svg'; | ||
| import DownArrow from '../../../assets/images/down.svg'; | ||
| import DownArrowSmall from '../../../assets/images/down-small.svg'; |
There was a problem hiding this comment.
I don't think you actually need a new asset for this. Why not just use the existing down arrow and just resize it to 12x12? That's what we have in the Figma file.
There was a problem hiding this comment.
Good point. I have removed the new asset and now reusing the old one in the latest commit.
Thanks.
…k/Expensify.cash into 3770-multi-language-improvements
madmax330
left a comment
madmax330
left a comment
There was a problem hiding this comment.
Looks good, just a couple of small comments I would like resolved before merging
| {size === 'small' | ||
| ? ( | ||
| <Icon | ||
| width={12} |
There was a problem hiding this comment.
@shawnborton should we set constants for these?
There was a problem hiding this comment.
Or should they be in the pickerstyles?
There was a problem hiding this comment.
+1 I think we put all styles inthe styles file, no?
There was a problem hiding this comment.
Also why we need this conditional, shouldn't that be handled here?
There was a problem hiding this comment.
should we set constants for these?
added styles.pickerSmall.icon.{width,heigth} and now using those instead of hardcoded values
Also why we need this conditional, shouldn't that be handled here?
Since this Picker component supports overwriting the Icon component via props and this is not a required prop, the default implementation for it is in Picker/PickerPropTypes.js. If we were do this logic in Picker/index.js we kinda have to ditch the icon prop entirely since it will be overwritten inside the component anyway. I too feel like it doesn't look good, but don't know if we can do it cleaner. Maybe we should indeed drop support for icon overwrite since it doesn't seem to be used, but I think we should look into why it was implemented like that in the first place, maybe it is required for some future feature?
src/components/LocalePicker/index.js
Outdated
| const localesToLanguages = { | ||
| default: { | ||
| value: 'en', | ||
| label: translate('preferencesPage.languages.english'), |
There was a problem hiding this comment.
I think we should keep the language name labels in their original Language. So English would always be "English" spanish will always be "Espagnol" and so on.
Otherwise say someone changes my app to chinese (or I do by accident) I'm going to have a tough time getting it back to english.
There was a problem hiding this comment.
Yeah, I think I agree.
So maybe what we want to do is:
- Add a translation for
languageNamethat we add to each translation file - Do not use the translate method from withLocalize here (but we do want to use it for the label in line 55) and instead use the translate method directly from the translate file
- Pass the as 1st param the locale of the item we are showing (ie: 'en' here and 'es' below)
That way, we ensure we have one language name per language and that each language name shows in their own language.
There was a problem hiding this comment.
I totally agree, I too get into this kinda situation from time to time.
@iwiznia I have implemented the change, but I completely missed the idea behind your first point. Please take a look at the current version of code and let me know if there is something to be added.
Thanks.
| @@ -0,0 +1,86 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Commenting here because I can't comment on the file name itself. I think this file should be src/components/LocalePicker.js, no? Since we only have one version of it, we don't need the folder, right?
There was a problem hiding this comment.
You're right. I have moved components/LocalePicker/index.js to components/LocalePicker.js. Thanks.
src/components/LocalePicker/index.js
Outdated
| const localesToLanguages = { | ||
| default: { | ||
| value: 'en', | ||
| label: translate('preferencesPage.languages.english'), |
There was a problem hiding this comment.
Yeah, I think I agree.
So maybe what we want to do is:
- Add a translation for
languageNamethat we add to each translation file - Do not use the translate method from withLocalize here (but we do want to use it for the label in line 55) and instead use the translate method directly from the translate file
- Pass the as 1st param the locale of the item we are showing (ie: 'en' here and 'es' below)
That way, we ensure we have one language name per language and that each language name shows in their own language.
src/components/LocalePicker/index.js
Outdated
| }, | ||
| es: { | ||
| value: 'es', | ||
| label: translate('preferencesPage.languages.spanish'), |
There was a problem hiding this comment.
| label: translate('preferencesPage.languages.spanish'), | |
| label: translate('preferencesPage.languages.es'), |
Same for the above
There was a problem hiding this comment.
Sorry, but I'm confused about this one. Do you want me to change the translation keys for languages to be their 2 letter code instead of full language name (spanish -> es, english -> en)? I thought your point about es and en was to pass them as first argument to the translate function.
| return ( | ||
| <> | ||
| {size === 'normal' && ( | ||
| <Text style={[styles.formLabel]} numberOfLines={1}> |
There was a problem hiding this comment.
| <Text style={[styles.formLabel]} numberOfLines={1}> | |
| <Text style={[styles.formLabel]} numberOfLines=1> |
Is this valid and equivalent?
There was a problem hiding this comment.
No, a JSX value should be either an expression or a quoted JSX text.
| {size === 'small' | ||
| ? ( | ||
| <Icon | ||
| width={12} |
There was a problem hiding this comment.
+1 I think we put all styles inthe styles file, no?
| {size === 'small' | ||
| ? ( | ||
| <Icon | ||
| width={12} |
There was a problem hiding this comment.
Also why we need this conditional, shouldn't that be handled here?
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging in version: 1.0.80-3🚀
|
|
This was broken and fixed here #4286 thought it is possible the problem came from other PRs written while this was being reviewed. |
|
🚀 Deployed to production in version: 1.0.81-4🚀
|
5 participants
Details
This PR makes these changes:
internationalizationbeta.LocalePickerso it can be reused across the app.LocalePickerto sign in page's footer.Fixed Issues
$ #3770
Tests
QA Steps
Tested On
Screenshots
Web
3770-web.mp4
Mobile Web
Simulator.Screen.Recording.-.iPhone.12.-.2021-07-21.at.15.07.17.mp4
Desktop
Screen.Recording.2021-07-21.at.15.01.01.mov
iOS
Simulator.Screen.Recording.-.iPhone.12.-.2021-07-21.at.15.10.28.mp4
Android
3770-android.mp4