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

Issue 331 migrate json language catalogs to po #366

Conversation

bram-atmire
Copy link
Member

No description provided.

Copy link
Contributor

@paulo-graca paulo-graca left a comment

Choose a reason for hiding this comment

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

Thank you @bram-atmire for adding this! I think your PR is a little far behind and needs to be synchronized to the latest version.

src/app/shared/lang-switch/lang-switch.component.spec.ts Outdated Show resolved Hide resolved
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

There are a few escaping issues in the labels. Also, because the metadata as a map PR isn't merged in this branch yet, it's hard to test, because nothing using metadata will work properly.

@bram-atmire bram-atmire force-pushed the issue-331-migrate-json-language-catalogs-to-po branch from 94f2d6f to e6c1545 Compare March 11, 2019 21:54
@bram-atmire bram-atmire force-pushed the issue-331-migrate-json-language-catalogs-to-po branch from e6c1545 to f4dae47 Compare April 7, 2019 11:49
@bram-atmire
Copy link
Member Author

@artlowel @paulo-graca
Normally the build should now work and the tests should pass. I also made another rebase and made sure that both dspace.pot (the template file), as well as en.po (the English translations) are up to date with the latest keys in en.json.

To help with the transition, I made sure that:

  • the original key name/path from en.json is available in .po and .pot comments
  • the ORDER of the keys corresponds with the order in en.json to the extent possible.

Can we identify any remaining blockers for stopping adding new keys to en.json? Would really love to see if we can finalize the transition, so we can start on documentation and onboarding of translators and developers with the new way of working with translations.

Dipped into that "Poedit" software for managing the translations, and it really looks like a great tool that will make life a lot easier for translators.

Copy link
Contributor

@AlexanderS AlexanderS left a comment

Choose a reason for hiding this comment

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

I found some minor issues, but overall this PR looks great.

author Bram Luyten <bram@atmire.com> 1542525711 +0100
committer Bram Luyten <bram@atmire.com> 1558347641 +0200

Removal of message keys and introduction of PO translation formats
@bram-atmire bram-atmire force-pushed the issue-331-migrate-json-language-catalogs-to-po branch from 1e57be1 to dadc4a7 Compare May 20, 2019 10:22
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 I have not tested this, but I reviewed the changes and I approve of this direction. Once merged, I'd recommend we start adding details/hints/tips for how to contribute new *.po files to these early docs: https://wiki.duraspace.org/display/DSPACE/DSpace+7+Translations (thanks for starting this @bram-atmire !)

Copy link
Contributor

@paulo-graca paulo-graca left a comment

Choose a reason for hiding this comment

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

Again, thank you @bram-atmire for adding this. It was very simple to me to add a new Language and add some translations. It works fine!

@AlexanderS
Copy link
Contributor

@bram-atmire Thanks for fixing my comments. Just a note: The current commit message looks a bit strange.

@tdonohue We could check if DSpace would qualify for the free open source tier of Transifex: https://docs.transifex.com/projects/open-source-project This would make it really easy to contribute new translations (there is a nice web editor).

@bram-atmire
Copy link
Member Author

bram-atmire commented May 30, 2019

We still need to find a solution for dynamic built keys like like:

app/+admin/admin-registries/metadata-schema/metadata-field-form/metadata-field-form.component.html:4:    <h4>{{messagePrefix + '.create' | translate}}</h4>
app/+admin/admin-registries/metadata-schema/metadata-field-form/metadata-field-form.component.html:8:    <h4>{{messagePrefix + '.edit' | translate}}</h4>
app/shared/object-list/search-result-list-element/item-search-result/item-search-result-list-element.component.html:2:  <span class="badge badge-light">{{ type.toLowerCase() + '.listelement.badge' | translate }}</span>
app/shared/dso-selector/modal-wrappers/create-community-parent-selector/create-community-parent-selector.component.html:2:    <div class="modal-header">{{'dso-selector.'+ action + '.' + objectType.toString().toLowerCase() + '.head' | translate}}
app/shared/dso-selector/modal-wrappers/dso-selector-modal-wrapper.component.html:2:  <div class="modal-header">{{'dso-selector.'+ action + '.' + objectType.toString().toLowerCase() + '.head' | translate}}
app/+item-page/edit-item-page/item-operation/item-operation.component.html:3:        {{'item.edit.tabs.status.buttons.' + operation.operationKey + '.label' | translate}}
app/+item-page/edit-item-page/item-operation/item-operation.component.html:8:    {{'item.edit.tabs.status.buttons.' + operation.operationKey + '.button' | translate}}
app/+item-page/edit-item-page/item-operation/item-operation.component.html:13:    {{'item.edit.tabs.status.buttons.' + operation.operationKey + '.button' | translate}}
app/+item-page/edit-item-page/edit-item-page.component.html:11:                            {{'item.edit.tabs.' + page + '.head' | translate}}
app/+item-page/simple/field-components/specific-field/title/item-page-title-field.component.html:3:    {{ type.toLowerCase() + '.page.titleprefix' | translate }}

@artlowel
Copy link
Member

artlowel commented Jul 25, 2019

Because the translation loader this PR is built on isn't actively maintained, has no support for server side rendering, and because the main developer behind ngx-translate isn't quite sure how much support he can give the project in the future, we decided in the meeting of 2019-07-25 to close this PR, and keep working with ngx-translate's native format for the time being.

To make translating easier for non-developers we'll create a PR to flatten the existing json files. We'll also look in to ways of converting the json files to po or xliff format and back, so those formats can still be used by translators, just not live in the application.

@artlowel artlowel closed this Jul 25, 2019
@tdonohue
Copy link
Member

This PR is currently replaced by #439. More thoughts/opinions welcome there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants