[RN] Fix internationalization on mobile #2318
Conversation
|
I think I found the issue. i18next returns different locale format names of what momentjs requires. This also leads to a crash I found today, when the React native app runs in prod mode. Added it to Jira, will fix it first thing. |
|
Does it crash with the first commit applied? I didn’t run in release mode though... |
|
Haven't checked it with this PR yet, i found the crash with the master. |
|
@paweldomas I believe @lyubomir wanted to have a look at the 2nd commit, but you should be able to merge the first, since current master is broken on non english locales, and this fixes it. |
|
Rebased + force pushed now that the first commit landed. The remaining commit just adds the builtin translations. |
|
@lyubomir Do you still want to take a look at this? |
| { | ||
| name: 'bg', | ||
| mainResource: BG_MAIN, | ||
| langResource: BG_LANG |
There was a problem hiding this comment.
@saghul, I'm afraid I don't understand the purpose of langResource! I don't see it used anywhere (in the file given that languages is not exported out of the file).
There was a problem hiding this comment.
If langResource is not necessary, could we please maybe simplify the source code? Like maybe converting the languages array to a map, the name to a key in the map, and mainResource to the value associated with the key that's require?
const languages = {
bg: require('../../../../lang/main-bg')
}There was a problem hiding this comment.
It is necessary, I just made a mistake :-( it should go here: https://github.com/jitsi/jitsi-meet/pull/2318/files#diff-c2a31f269ba5f30dfabe9049972beafaR196
There was a problem hiding this comment.
I'll fix that, and see if we can make this any simpler. 👍
react/features/base/i18n/i18next.js
Outdated
| if (BuiltinLanguages && BuiltinLanguages.setup) { | ||
| BuiltinLanguages.setup(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Is order really important here? Because if order isn't really necessary, we might as well simplify this by not exporting setup from BuiltinLanguages and doing the whole initialization in BuiltinLanguages.native.js.
|
@lyubomir I simplified things a bit, PTAL. I don't think we can go full dynamic, because the React Native packager won't be able to figure out what to bundle: facebook/react-native#6391 |
|
Ah, wait, there is one more thing I can do! Turn those imports into requires at the map declaration phase. That works. |
Load all of them as imports, so the packager includes them in the bundle. Then register them with the i18next library.
|
@lyubomir Now! :-) |
Only the first commit is necessary to fix the Android crash, but I thought I'd try to add actual internationalization support, hence commit number 2.
I'd like to hear thoughts on the implementation :-)
Here is how it looks like in Spanish screenshot is on Android, but I also tested iOS):
As you can see, dates are not translated, maybe @zbettenbuk can take a look at that.