-
Notifications
You must be signed in to change notification settings - Fork 3
Switch to a simple gettext l10n system #113
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
Conversation
|
You ment to make us all review this not work on it, right? |
I meant to have you work on the review :) |
Signed-off-by: Christoph Wurst <[email protected]>
2da9b3c to
cd955d0
Compare
| LOCALES.map(data => { | ||
| gt.addTranslations(data.locale, 'messages', data.json) | ||
| }) | ||
| gt.setLocale(locale) |
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.
Shouldn't we use the language settings for this instead of the locale setting?
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.
I'm never sure. @nickvergessen knows best
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.
Well see nextcloud/server#14784
So moment JS is actually "fucked up" as it has only one option to set this and is unable to handle a locale being different from the 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.
OK, then it should be fine for now otherwise it would just be the "seconds" which would be in the proper language, the rest provided by moment would have the language defined by the locale.
This seems like a trivial change. But it took me ages to figure out. Anyway …
What it does is
For technical reasons I had to switch from a simple Babel+Typescript system to one with Webpack in order to do the locale bundling. This also forced me to switch from Jest to Mochapack. But that works just as fine. The tests here are trivial.
Once https://docs.transifex.com/transifex-github-integrations/github-tx-ui is set up (having my fingers crossed that this actually works), I'll add some integration tests for different locales. But manually adding an en.pot allowed me to test translation successfully.
@nextcloud/javascript please have a look. I would like to use this approach for https://github.com/nextcloud/nextcloud-vue as well.
Fixes #1