Skip to content
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

refactor: translate() to t() #777

Merged
merged 16 commits into from
Apr 25, 2018
Merged

Conversation

machour
Copy link
Member

@machour machour commented Apr 19, 2018

Description

Wrote a babel-traverse script that took care of migrating all calls from translate() to t() swapping keys with strings everywhere.

Then did some polishing for the relativeTimeFromNow() function & fixed a few things in t().

Also removed untranslated english sentences from translation files.

Should be good to go!

Next thing will be to go over some of the old concatenations we're currently doing and turn those into plain sentences (like I did for the events screen)

@coveralls
Copy link

coveralls commented Apr 19, 2018

Coverage Status

Coverage increased (+0.1%) to 41.7% when pulling 29f5c32 on machour:refactor/translate-to-t into 777ac29 on gitpoint:master.

if (typeof interpolation[match[2]] === 'undefined') {
ongoing += this.config.missingPlaceholder;
} else if (typeof interpolation[match[2]] === 'object') {
const value = interpolation[match[2]];
Copy link
Member

@chinesedfan chinesedfan Apr 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also add a local variable to save typeof value. And how about to replace the while loop with translation.replace(componentPlaceholdersReg, (match) => { /* blabla */ })?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for the local typeof value const.

I didn't know that replace() also accepted a callback as a second parameter, named parameters greatly improve code readability, thank you!

@machour
Copy link
Member Author

machour commented Apr 23, 2018

@chinesedfan could you pull this PR and check if everything is okay for zh? 🙏
Since this PR touches a lot of files, I'd love to get it merged to avoid breaking it with other merges.

@housseindjirdeh
Copy link
Member

Beautiful :)

If you would like me to pull it down and review locally as well, let me know - I can do that shortly if need be 🙏

@chinesedfan
Copy link
Member

@machour I noticed a bug due to I18n.defaultSeparator === '.'. Sentences include the dot (like You may have to request approval for them., which appears in the bottom of auth-profile.screen) can not be translated correctly.

2018-04-25 11 01 17

@machour
Copy link
Member Author

machour commented Apr 25, 2018

@chinesedfan Thank you for spotting this!

Fixed by setting I18n.defaultSeparator = 'GITPOINT-DISABLED';

@machour
Copy link
Member Author

machour commented Apr 25, 2018

@housseindjirdeh yes please! Absolutely nothing should change on the UI side with this PR.

This kind of renders react-native-i18n useless (see #783) and will make switching to lingui-js or other more translator friendly libraries painless in the future :)

I'll add some documentation for translation handling once this is merged.

@housseindjirdeh
Copy link
Member

Perused through the app, and couldn't find any translation files missing/broken 👍

@machour machour merged commit 80b4fe9 into gitpoint:master Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants