Skip to content

fix(trans): Make all translations contain same fields #478

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

Merged
merged 8 commits into from
Oct 27, 2017

Conversation

chinesedfan
Copy link
Member

I use a script to find out these missing fields.

How about adding a translations check task in Travis CI?

@lex111
Copy link
Member

lex111 commented Oct 15, 2017

How about adding a translations check task in Travis CI?

It would be good to check it, but how did you do it?

@chinesedfan
Copy link
Member Author

@lex111 By a Node.js script, it is easy. You can image it as a test suite file.

@chinesedfan
Copy link
Member Author

I just integrated the check to travis ci.

You can find that es is invalid. I didn't fix it because #442 will do that.

@lex111
Copy link
Member

lex111 commented Oct 15, 2017

@chinesedfan great and useful script! Can invite translators here?

@chinesedfan
Copy link
Member Author

chinesedfan commented Oct 15, 2017

Hey guys, I need your help here. Can you translate those missing fields, please? Of course, anybody can help even not mentioned below.

By the way, we'd better maintain translators list somewhere.

@abdurrahmanekr
Copy link
Contributor

Could you create a word list? Like #408

@chinesedfan
Copy link
Member Author

@abdurrahmanekr The missing words are different for different languages. Can you comment directly? For example, tr requires them.

@machour
Copy link
Member

machour commented Oct 15, 2017

@chinesedfan awesome work!

If we make this part of the travis check, could we define the guidelines that new translations should be added as "TRANSLATE_ME" instead of the english sentence or a poorly google translated string ?
This would make the translator life easier allowing him to directly spot untranslated strings.

Or we can have a second script running in pre-commit that does that automatically.

The translators list is already somehow available in CONTRIBUTORS.md. Translators have the translation key/emoji set

@machour
Copy link
Member

machour commented Oct 15, 2017

And here are the missing translations for fr:

 noUsersFound: 'Aucun utilisateur trouvé :(',
noRepositoriesFound: 'Aucun dépôt trouvé :(',
starred: 'Favoris',
watching: 'Abonné',
watchers: 'Abonnés',

(@Antoine38660, watcher = abonné sounds ok to you?)

@abdurrahmanekr
Copy link
Contributor

👍 @chinesedfan , here are the missing translations for TR:

noUsersFound: 'Kullanıcı Bulunamadı :('
noRepositoriesFound: 'Repository Bulunamadı'
watchers: 'İzleyenler'

@chinesedfan
Copy link
Member Author

@machour Marking untranslated strings as TRANSLATE_ME really makes it easy to be discovered. But it may also make the app displays wired.

I know we can find translators in CONTRIBUTORS.md, but don't know who is good at which language.

@abdurrahmanekr Thanks again!

@machour
Copy link
Member

machour commented Oct 15, 2017

@chinesedfan it makes the app display weird only when browsing it in translated mode using master. Which is mostly used by people who actually can translate the missing things they spot.

This would also allow to perform a quick check for releases: if there's a TRANSLATE_ME somewhere, we're not ready to release. (and if the translator is unavailable, then we can swap with an english string just for the release)

Seems like a fair trade-off when dealing with quality translations.

Copy link
Member

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Russian translation

@@ -147,6 +147,8 @@ export const ru = {
searchMessage: 'Поиск {{type}}',
repository: 'репозиториев',
user: 'пользователей',
noUsersFound: 'No users found :(',
Copy link
Member

Choose a reason for hiding this comment

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

Пользователи не найдены :(

Copy link
Member Author

Choose a reason for hiding this comment

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

@lex111 Updated. Thank you!

@@ -147,6 +147,8 @@ export const ru = {
searchMessage: 'Поиск {{type}}',
repository: 'репозиториев',
user: 'пользователей',
noUsersFound: 'No users found :(',
noRepositoriesFound: 'No repositories found :(',
Copy link
Member

Choose a reason for hiding this comment

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

Репозитории не найдены :(

@@ -257,14 +259,19 @@ export const ru = {
areYouSurePrompt: 'Вы уверены?',
applyLabelTitle: 'Добавить ярлык к этой задаче',
},
comment: {
commentActions: 'Comment Actions',
Copy link
Member

Choose a reason for hiding this comment

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

Действия с комментарием

@@ -257,14 +259,19 @@ export const ru = {
areYouSurePrompt: 'Вы уверены?',
applyLabelTitle: 'Добавить ярлык к этой задаче',
},
comment: {
commentActions: 'Comment Actions',
editCommentTitle: 'Edit Comment',
Copy link
Member

Choose a reason for hiding this comment

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

Редактирование комментария

comment: {
commentActions: 'Comment Actions',
editCommentTitle: 'Edit Comment',
editAction: 'Edit',
Copy link
Member

Choose a reason for hiding this comment

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

Редактировать

commentActions: 'Comment Actions',
editCommentTitle: 'Edit Comment',
editAction: 'Edit',
deleteAction: 'Delete',
Copy link
Member

Choose a reason for hiding this comment

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

Удалить

@peterblazejewicz
Copy link
Contributor

Hey guys, I need your help here. Can you translate those missing fields, please? Of course, anybody can help even not mentioned below.

@chinesedfan see #481

andrewda pushed a commit that referenced this pull request Oct 15, 2017
* fix(trans): Correct Polish translation wording

- typos
- changed wording to work better in UI context

* feat(trans): New resource keys translation. See #478
@@ -243,6 +243,7 @@ export const de = {
membersTitle: 'MITGLIEDER',
descriptionTitle: 'BESCHREIBUNG',
},
organizationActions: 'Organization Actions',
Copy link
Contributor

Choose a reason for hiding this comment

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

Organisation Aktionen

Copy link
Member Author

Choose a reason for hiding this comment

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

@tautf 👍

@@ -287,6 +288,7 @@ export const de = {
openIssueSubTitle: '#{{number}} wude vor {{time}} von {{user}} geöffnet',
closedIssueSubTitle:
'#{{number}} von {{user}} wurde vor {{time}} geschlossen',
issueActions: 'Issue Actions',
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue Aktionen

@@ -343,5 +345,6 @@ export const de = {
abbreviations: {
thousand: 't',
},
openInBrowser: 'Open in Browser',
Copy link
Contributor

Choose a reason for hiding this comment

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

In Browser öffnen

@Antoine38660
Copy link
Member

@machour yes... watchers can be abonnés (but looks bizarre 😉)

@jouderianjr
Copy link
Member

@chinesedfan using k in brazilian portuguese for thousand looks fine for me 😄

@nersoh @arthurdenner I know you guys are Brazilians also, what do you think about?

@arthurdenner
Copy link
Contributor

I think it's fine as well. We usually use k to refer to thousands.

@housseindjirdeh
Copy link
Member

Thank you all for this <3

@chinesedfan definitely would love to have a translations script check as part of our CI pipeline in travis.yml. Please feel free to put up a separate PR for that

@chinesedfan
Copy link
Member Author

@housseindjirdeh I already integrated the check script in this PR. But if you persist, I can separate it out.

.travis.yml Outdated
@@ -9,3 +9,4 @@ before_script:
- 'yarn'
script:
- yarn run eslint
- node __tests__/locales.test.js
Copy link
Member

Choose a reason for hiding this comment

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

Probably shouldn't do this here – this is going to cause a lot off issues with #407 and will end up just being moved out. Try to refactor this to the tests branch and make a PR there and it'll show up in #407. I can definitely help out in this area.

Copy link
Member Author

Choose a reason for hiding this comment

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

@andrewda I picked out commit 66b3228 and kept this PR focus on translations.

Do you have any suggestions for where I can put locales.test.js? I think __tests__ is not a good place, because it only contains jest testing files.

@chinesedfan chinesedfan removed the tests label Oct 20, 2017
@andrewda andrewda mentioned this pull request Oct 20, 2017
63 tasks
@chinesedfan
Copy link
Member Author

Can we merge this PR first? It is hard to find translators for eo/nl, and we used to keep words in English if no one can help. @housseindjirdeh @lex111

@machour
Copy link
Member

machour commented Oct 26, 2017

+1 on moving forward with this and have Travis as a goal keeper for translations keys.

@chinesedfan chinesedfan merged commit 3714dc5 into gitpoint:master Oct 27, 2017
@chinesedfan chinesedfan deleted the fixtrans branch October 27, 2017 10:58
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.