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

Big translations update - Ready to review #376

Merged
merged 77 commits into from Dec 5, 2017

Conversation

Projects
None yet
4 participants
@mtancoigne
Contributor

mtancoigne commented Nov 23, 2017

As stated in #185, the current state of the translations lacks some elements.
I started to check every view one by one and replace the hardcoded string by translated ones.

We're working on this for our next conferences, so the changes are live on our server: https://evenements.agilemans.org

Here is the checklist:

  • - Translate views
  • - Override view from Devise and make them translatable
  • - Translate strings in controllers
  • - Translate strings in helpers
  • - Translate activerecord's fields names
  • - Automatic language selection from #379

For now, we think that most of the translation work is done; if you're interested by this PR, tell me and I'll dispatch the new strings in existing files

  • - Re-order strings in translations files

@mtancoigne mtancoigne changed the title from Big translations update - WIP to Big translations update - Ready to merge Nov 30, 2017

@mtancoigne mtancoigne changed the title from Big translations update - Ready to merge to Big translations update - Ready to review Nov 30, 2017

@erdgeist

Impressive work. I reviewed for obvious problems in change to execution, but most diffs are replacing static strings with translation macros. Rest looks good.

@erdgeist

This comment has been minimized.

Show comment
Hide comment
@erdgeist

erdgeist Dec 2, 2017

Contributor

I would love to see this merged. Then I'd open another PR with German localization and fix up my conflicting PRs.

Contributor

erdgeist commented Dec 2, 2017

I would love to see this merged. Then I'd open another PR with German localization and fix up my conflicting PRs.

@mtancoigne

This comment has been minimized.

Show comment
Hide comment
@mtancoigne

mtancoigne Dec 2, 2017

Contributor

By the way, thanks for the review :)

Contributor

mtancoigne commented Dec 2, 2017

By the way, thanks for the review :)

@mtancoigne

This comment has been minimized.

Show comment
Hide comment
@mtancoigne

mtancoigne Dec 3, 2017

Contributor

Well then... dictionnary.com proposes these forms:

Existing - Related forms
exister, noun
nonexisting, adjective
unexisting, adjective

So I guess unexisting is good too :) I'm not that good in english and english history of words to argue, so if you think this should be unexisting, then unexisting it will be.

Contributor

mtancoigne commented Dec 3, 2017

Well then... dictionnary.com proposes these forms:

Existing - Related forms
exister, noun
nonexisting, adjective
unexisting, adjective

So I guess unexisting is good too :) I'm not that good in english and english history of words to argue, so if you think this should be unexisting, then unexisting it will be.

@manno manno merged commit 1f57fb2 into frab:master Dec 5, 2017

2 checks passed

VersionEye All software dependencies are fine. You are awesome!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@smortex

This comment has been minimized.

Show comment
Hide comment
@smortex

smortex Dec 5, 2017

Contributor

🎉

Contributor

smortex commented Dec 5, 2017

🎉

@mtancoigne mtancoigne deleted the el-cms:translations-fr branch Dec 5, 2017

@erdgeist

This comment has been minimized.

Show comment
Hide comment
@erdgeist

erdgeist Dec 6, 2017

Contributor

Would you mind providing the last step: "Re-order strings in translations files"? There's translations missing now, at least in German that need to be fixed.

Contributor

erdgeist commented Dec 6, 2017

Would you mind providing the last step: "Re-order strings in translations files"? There's translations missing now, at least in German that need to be fixed.

@mtancoigne

This comment has been minimized.

Show comment
Hide comment
@mtancoigne

mtancoigne Dec 7, 2017

Contributor

No problem, i'll do it for monday!

Contributor

mtancoigne commented Dec 7, 2017

No problem, i'll do it for monday!

@mtancoigne

This comment has been minimized.

Show comment
Hide comment
@mtancoigne

mtancoigne Dec 9, 2017

Contributor

@erdgeist, @smortex : I'm working on creating good translations files, and I have two options: split all the string in small files, like it is now but with more files, or create one, big translation file per language.

The first solution would make the translations pretty easy to find as long as they are well maintained, and the second one would make a big file (1547 lines) but easier to diff with other language files, and easier to maintain...

What do you think ?

Contributor

mtancoigne commented Dec 9, 2017

@erdgeist, @smortex : I'm working on creating good translations files, and I have two options: split all the string in small files, like it is now but with more files, or create one, big translation file per language.

The first solution would make the translations pretty easy to find as long as they are well maintained, and the second one would make a big file (1547 lines) but easier to diff with other language files, and easier to maintain...

What do you think ?

@erdgeist

This comment has been minimized.

Show comment
Hide comment
@erdgeist

erdgeist Dec 10, 2017

Contributor

As long as the templates are there to easily add new translations, I'm fine with either way. I think diffing a large file is not harder than with a directory.

Contributor

erdgeist commented Dec 10, 2017

As long as the templates are there to easily add new translations, I'm fine with either way. I think diffing a large file is not harder than with a directory.

@smortex

This comment has been minimized.

Show comment
Hide comment
@smortex

smortex Dec 10, 2017

Contributor

Hello

I share @erdgeist point of view, it does not change much. A single file can be more usefull if some blocks of translations are reused, but a) it's not the case currently and b) I am not certain this is an anti-pattern.

Maintaining the translations sorted the same way in different locales allows easier translations (if number of lines are different, translations are missing; graphical diff tools such as "meld" make translating things quite easy).

Contributor

smortex commented Dec 10, 2017

Hello

I share @erdgeist point of view, it does not change much. A single file can be more usefull if some blocks of translations are reused, but a) it's not the case currently and b) I am not certain this is an anti-pattern.

Maintaining the translations sorted the same way in different locales allows easier translations (if number of lines are different, translations are missing; graphical diff tools such as "meld" make translating things quite easy).

@mtancoigne

This comment has been minimized.

Show comment
Hide comment
@mtancoigne

mtancoigne Dec 10, 2017

Contributor

Ok, I agree on splitting over multiple files. I'm doing it this way:

locales/
  <lang>/
   <key>.yml

I updated application.rb accordingly to load the translations in the localized folder instead of the file name, so translated files will all have the same names (instead of <module>.<locale>.yml)

It's a lot of work, but I hope i'll have it done for tomorrow.

Contributor

mtancoigne commented Dec 10, 2017

Ok, I agree on splitting over multiple files. I'm doing it this way:

locales/
  <lang>/
   <key>.yml

I updated application.rb accordingly to load the translations in the localized folder instead of the file name, so translated files will all have the same names (instead of <module>.<locale>.yml)

It's a lot of work, but I hope i'll have it done for tomorrow.

@smortex

This comment has been minimized.

Show comment
Hide comment
@smortex

smortex Dec 10, 2017

Contributor
Contributor

smortex commented Dec 10, 2017

@mtancoigne

This comment has been minimized.

Show comment
Hide comment
@mtancoigne

mtancoigne Dec 10, 2017

Contributor

Yes, there's one : that's the first rail app in which I stick my keyboard :D

So you think i should keep the current file naming rule (<module>.<lang>.yml) instead ?
The problem I can see with that is the following: there are 41 keys in the new french translations (and they are not even merged with keys/string from es/de/pt-BR, so it should grow). That means 41 files with the .fr.yml extension. Times 5 languages : 205 files in a single folder. Putting them in a subfolder per locale, with no locale in the name would make it really easier to diff/maintain (i.e. : meld en fr).

If there is a better thing to do, i'd be happy to know it :)

Contributor

mtancoigne commented Dec 10, 2017

Yes, there's one : that's the first rail app in which I stick my keyboard :D

So you think i should keep the current file naming rule (<module>.<lang>.yml) instead ?
The problem I can see with that is the following: there are 41 keys in the new french translations (and they are not even merged with keys/string from es/de/pt-BR, so it should grow). That means 41 files with the .fr.yml extension. Times 5 languages : 205 files in a single folder. Putting them in a subfolder per locale, with no locale in the name would make it really easier to diff/maintain (i.e. : meld en fr).

If there is a better thing to do, i'd be happy to know it :)

@smortex

This comment has been minimized.

Show comment
Hide comment
@smortex

smortex Dec 10, 2017

Contributor
Contributor

smortex commented Dec 10, 2017

@mtancoigne

This comment has been minimized.

Show comment
Hide comment
@mtancoigne

mtancoigne Dec 10, 2017

Contributor

I though that managing this tree of files required modifications from Rails point of view
No, I only added this line in the application_controller.rb to change the locales dirs:

    supported_languages.each { |l| I18n.load_path += Dir[Rails.root.join('config', 'locales', l, '*.yml').to_s] }

I'll open a PR as soon as there is something to show off, i'm sorting spaghettis for now.

Contributor

mtancoigne commented Dec 10, 2017

I though that managing this tree of files required modifications from Rails point of view
No, I only added this line in the application_controller.rb to change the locales dirs:

    supported_languages.each { |l| I18n.load_path += Dir[Rails.root.join('config', 'locales', l, '*.yml').to_s] }

I'll open a PR as soon as there is something to show off, i'm sorting spaghettis for now.

@mtancoigne mtancoigne referenced this pull request Dec 10, 2017

Closed

Translations - WIP #382

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment