-
Notifications
You must be signed in to change notification settings - Fork 37
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
Feature/i18n #249
base: main
Are you sure you want to change the base?
Feature/i18n #249
Conversation
Thanks for the PR, awesome work! We've been planing to do it soon, so good to have someone help out with it. The code looks good, I'll test it and then approve if everything works. |
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 works well, and I like how you implemented it, just a few minor details:
- Please move the "Language" settings between "Appearance" and "Push notifications" - I think it's logical for it to be in the section that allows you to "customize" the app
- when selecting a language (under the "Language" menu entry you created) would it be possible to show each language's name in its native language? (ex. English always shows up as "English" and Portuguese (Brazil) always shows up as "Portugues (Brasil)") I think this is how most apps do it, and it's also how it works in Android system language picker
- Do you know if it's possible to support per-app languages through system settings on Android and iOS
It would make sense to reuse the language files from the server / web version: https://github.com/pixelfed/pixelfed/blob/dev/crowdin.yml as they are already translated on crowdin: https://crowdin.com/project/pixelfed I'm sure crowdin offers some way to pull/download the translations that is compatible with the common translation libraries. |
I've walked to @dansup and we'll get separate translations. I think it will be cleaner that way |
perfect!
|
that would mean more strings to translate, which means less completion overall because translators are not paid. speaking from experience in delta chat it makes sense to have one translation repo. for strings that don't match you can have strings that are just used on web or on desktop. |
I see your point. Let's see if we can reuse web strings when we get to the task of properly supporting other languages. I'd keep this PR as-is anyway, so we have something that we can build on in the future. |
…translation management
…e translation handling
…ved locale display
Regarding the 3rd point looks like this package does the job: But looks like lacking timezone support to get the device timezone, that makes things easier when you want to show for ex. "3 hours ago" already formatted in the best way end translated. |
Thanks for doing the research. If it requires a separate package, then let's go without that. |
…e device locale detection
Just made the adjustments @Kwasow The current state with the adjustments |
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 good to me now! Thank you for your contribution. We will merge your changes soon.
The web version has translations, but the app isn't right now. Even if it is only available in English, it would be nice to make the app ready to translate in the future.
In my opinion, it is better to do it as soon as possible when fewer screens and components are done to avoid a bigger refactor in the future
This can be done easily using
expo-localization
(https://docs.expo.dev/versions/latest/sdk/localization/) andi18n-js
(https://www.npmjs.com/package/i18n-js).I made some tests adding the i18n lib, the translation files for
en
andpt-BR
, and refactoring the login and settings screens to use itTest on Android device
Test on iOS Simulator
closes #174