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.

Sign up for GitHub

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

Jump to bottom

i18n tooling and translation #27

Merged
merged 1 commit into from
Oct 4, 2018
Merged

i18n tooling and translation #27

merged 1 commit into from
Oct 4, 2018

Conversation

OmarIthawi
Copy link
Member

@OmarIthawi OmarIthawi commented Sep 5, 2018 •
edited
Loading

We'd like to add Japanese as a supported language, also supported JavaScript translations via
django-statici18n.

Before you continue, take a look at edx/i18n-tools and edx/cookiecutter-django-app.

Summary of the Changes

  • Added a Makefile to help manage the translations. The file is copied and modified from: edx/cookiecutter-django-app
  • Created a locale link to google_drive/translations to allow the i18n_tools find the po/mo files
  • Added google_drive/translations/config.yaml which works as the configuration file of the i18n_tools command
  • Added requirements-dev.txt to pin development packages. Used for now only for translations.

TODO

  • Support both django
    • djangojs translation domains is ignored because the only string there's is for testing purposes
  • Make Travis happy
  • Document how to update the strings properly from Transifex
  • Manually test that the dummy Esperanto translation is working
  • Support adding locale to the embed code via hl=ja_JP parameter e.g. 
    http://www.google.com/calendar/embed?src=jkloc408t9h5rdv686c8d567hs%40group.calendar.google.com&ctz=America/Chicago&hl=es
  • Add tests for the calendar localization ☝️

Screenshot

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Sep 5, 2018
@OmarIthawi OmarIthawi changed the title (WIP) i18n tooling and translation i18n tooling and translation Sep 5, 2018
@openedx openedx deleted a comment from openedx-webhooks Sep 5, 2018
@mduboseedx mduboseedx removed needs triage open-source-contribution PR author is not from Axim or 2U labels Sep 5, 2018
@OmarIthawi OmarIthawi force-pushed the omar/gdrive-i18n branch 2 times, most recently from db709c5 to 2a8f01f Compare September 11, 2018 22:46
@OmarIthawi
Copy link
Member Author

@mduboseedx Could you please let me know why the open-source-contribution label have been removed?

@mduboseedx
Copy link

mduboseedx commented Sep 12, 2018 •
edited
Loading

@OmarIthawi I actually deleted those labels but that may be due to my own confusion with the edx-solutions owner. I can easily add those back though

@OmarIthawi
Copy link
Member Author

Thanks @mduboseedx. I'll tag the latest author on this repo.

@OmarIthawi
Copy link
Member Author

@jmbowman Could you please take a look? or do you know someone else who could review this PR?

@OmarIthawi
Copy link
Member Author

Just a gentle reminder @jmbowman.

jmbowman
jmbowman reviewed Oct 1, 2018
View reviewed changes
Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed response; still catching up from vacation and hackathon.

Makefile Outdated Show resolved Hide resolved
google_drive/google_calendar.py
CALENDAR_TEMPLATE = "/templates/html/google_calendar.html"
CALENDAR_EDIT_TEMPLATE = "/templates/html/google_calendar_edit.html"


def _(text):
"""
Dummy ugettext.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is a stub function being used here instead of the actual ugettext function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, because XBlocks are initialized as soon as possible and that makes Django freaks out with an exception!

All the other XBlocks uses this trick to overcome this limitation. Honestly, this a pretty useless trick, because XBlocks don't yet support i18n for field name, so 🤷‍♂️

But everyone's doing this in the hope that we can do something about it soon, I'm also optimistic about that as well so I do the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Would ugettext_lazy work? https://docs.djangoproject.com/en/1.11/topics/i18n/translation/#lazy-translation If that breaks something in the depths of the XBlock machinery, I'm fine with this as is.

Copy link
Member Author

@OmarIthawi OmarIthawi Oct 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @jmbowman I would love to use an actual ugettext function, but it's currently not possible.

Last time I checked it with the Poll XBlock it was causing an exception from a function deep down in XBlock. I tried it on the Google Drive and worked, OK.

However, I'm not comfortable with this change, so I'd like to keep it as-is and worry about this problem later. It's not going to be useful anyway since the XBlock fields, to my best knowledge, are instantiated at the class level, so ugettext_lazy will be as good as ugettext_noop or my dummy function i.e useless.

requirements/dev.txt Outdated Show resolved Hide resolved
requirements/dev.txt Outdated Show resolved Hide resolved
@OmarIthawi
Copy link
Member Author

OmarIthawi commented Oct 3, 2018 •
edited
Loading

Thanks @jmbowman for the review! I've addressed your comment and added a couple of commits.

Could you please take a look? Let me know if you think everything is good so I can squash the commits.

jmbowman
jmbowman approved these changes Oct 3, 2018
View reviewed changes
Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

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

Sure, makes sense.

@OmarIthawi
Copy link
Member Author

Thanks @jmbowman! I've squashed the changes. Please feel free to merge.

@jmbowman jmbowman merged commit 52d4053 into openedx:master Oct 4, 2018
@OmarIthawi
Copy link
Member Author

Thank you again @jmbowman!

@OmarIthawi OmarIthawi mentioned this pull request Jul 25, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmbowman jmbowman jmbowman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@OmarIthawi @mduboseedx @jmbowman @openedx-webhooks