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

Restructure i18n files #200

Merged
merged 1 commit into from
Jul 16, 2018
Merged

Restructure i18n files #200

merged 1 commit into from
Jul 16, 2018

Conversation

mahyard
Copy link
Contributor

@mahyard mahyard commented Jun 30, 2018

Via this PR:

  1. Latest messages extracted
  2. "locale" directory added to the main app and added to package_data
  3. Underscore template files renamed with .underscore extension

Some changes to README.md may be necessary.

@pomegranited
Copy link
Member

Thank you for this PR @mahyard ! Please ping me when you're ready for review.

@mahyard
Copy link
Contributor Author

mahyard commented Jul 4, 2018

Hi Mrs. Vogel,
It's almost ready for you to review
I have only small changes in README.md
and a question about some unnecessary files I suggest to remove:

problem_builder/locale/<lang_code>/LC_MESSAGES/django-partial.po
problem_builder/locale/<lang_code>/LC_MESSAGES/djangojs-partial.po
problem_builder/locale/<lang_code>/LC_MESSAGES/underscore.po

for any language we want to have its translation, we just need to store 4 files. djnago and djangojs (both .po and .mo)
I don't know why they pushed these extra files and may I remove them or not?
thank you @pomegranited

@pomegranited pomegranited removed their request for review July 5, 2018 11:43
@mtyaka
Copy link
Member

mtyaka commented Jul 5, 2018

@mahyard It's fine to remove those files if they are not needed. I don't think translations ever worked correctly for this xblock.

The [XBlock documentation|http://edx.readthedocs.io/projects/xblock-tutorial/en/latest/edx_platform/edx_lms.html#internationalization-support] says that translations should be present in the /<package>/translations/<lang>/LC_MESSAGES/text.po though. The DnDv2 xblock seems to follow that approach: https://github.com/edx-solutions/xblock-drag-and-drop-v2/tree/master/drag_and_drop_v2/translations/en/LC_MESSAGES.

Please ping me when the PR is ready for review.

@mahyard mahyard changed the title WIP: Restructure i18n files Restructure i18n files Jul 12, 2018
@mahyard
Copy link
Contributor Author

mahyard commented Jul 12, 2018

Hi @mtyaka @pomegranited
It's ready to review

@mahyard mahyard closed this Jul 12, 2018
@mahyard mahyard reopened this Jul 12, 2018
@mtyaka
Copy link
Member

mtyaka commented Jul 16, 2018

Thank you @mahyard! The changes look good. I had some problems getting the string from the underscore file translated until I realized I have to run ./manage.py lms compilejsi18n -v3 -problem-builder and then collectstatic to get the translations into the global django JS translation file.

Note though that using the django and djangojs translation domains for strings in xblocks is not encouraged because it interferes with translations in edx-platform, although for a long time this was the only option for i18n in xblocks.

The new i18n machinery for xblocks uses an i18n service, however the i18n service was only recently implemented for django translation template tags, and support for translating JS strings still hasn't landed in edx-platform. If you're interested in how that will work, you can check out the work-in-progress on the Drag and Drop v2 xblock: https://github.com/edx-solutions/xblock-drag-and-drop-v2/pull/156/files.

Since the i18n does not yet support JS files, I am merging this PR as an intermediate solution, but we will want to switch to i18n service in the future.

👍

  • I tested this: I verified that the translations from underscore files work (after manually running compilejsi18n and collectstatic)
  • I read through the code
  • I checked for accessibility issues this PR does not change the UI
  • Includes documentation

@mtyaka mtyaka merged commit 49975f9 into open-craft:master Jul 16, 2018
@bradenmacdonald
Copy link
Member

bradenmacdonald commented Jul 30, 2018

@mtyaka

Since the i18n does not yet support JS files, I am merging this PR as an intermediate solution, but we will want to switch to i18n service in the future.

I thought that we had merged the JS i18n code onto the solutions fork, so we could be using that now, at least on that fork?

@pomegranited
Copy link
Member

@bradenmacdonald @mtyaka We merged zyegfryed/django-statici18n#41 to support JS i18n in XBlocks, but have only used it on DnDv2 so far (cf openedx/xblock-drag-and-drop-v2#156).

@mtyaka
Copy link
Member

mtyaka commented Jul 31, 2018

@bradenmacdonald Did you copy-paste a wrong quote? I don't see how completable-by-viewing code is relevant :)

@pomegranited Do we need to open a PR to update the version of django-statici18n in edx/edx-platform? Are there any other changes changes to edx/edx-platform and/or edx/XBlock required for namespaced JS translations to work?

@pomegranited
Copy link
Member

@mtyaka

Do we need to open a PR to update the version of django-statici18n in edx/edx-platform?

Not required -- it only needs to be used by the XBlock when generating the JS files shipped with that xblock, and so doesn't require the platform to use the same version.

Are there any other changes changes to edx/edx-platform and/or edx/XBlock required for namespaced JS translations to work?

Nope, the changes are isolated to the XBlock itself. Namespacing protects the platform's JS from the XBlock's specific translations, and only just changes how the XBlock access its own translations.

@mtyaka
Copy link
Member

mtyaka commented Jul 31, 2018

@pomegranited Oh excellent, thanks for the info!

@bradenmacdonald
Copy link
Member

@mtyaka lol whoops. I've edited my comment. I meant to reply to "Since the i18n does not yet support JS files, I am merging this PR as an intermediate solution, but we will want to switch to i18n service in the future."

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.

4 participants