search

openedx / xblock-image-explorer

GNU Affero General Public License v3.0
9 stars 39 forks source link

[MCKIN-2723] Needs i18n runtime service to provide translations for HTML templates #37

Closed pomegranited closed 6 years ago

pomegranited commented 6 years ago

Once the dependent PRs are merged, this change is all that's required to allow this XBlock to use its own translations for text found in HTML templates.

JIRA tickets: MCKIN-7023

Discussions: WL-230, YONK-879

Dependencies:

Screenshots:

Before these changes, the only text in Image Explorer HTML templates that would appear translated was the text that happened to be translated in edx-platform. before

After this change, Image Explorer template translations like this one appear in Studio and LMS: after

Sandbox URL: sandbox is being provisioned.

Merge deadline: None

Testing instructions:

  1. Ensure that this xblock is installed (it's not included by default).
  2. In Studio, create a course and add "image-explorer" to the Advanced Settings > Advanced Module List. Add an Image Explorer unit, e.g. Sandbox Image Explorer course.
  3. Visit the Studio URL with /update_lang appended, and submit eo as your selected Language Code.
  4. Edit the Image Explorer unit, and note that trans template tags are respected, and their strings appear translated.

See the updated README and run_test.py for details on running the automated tests.

Reviewers

bradenmacdonald commented 6 years ago

Can we change from render_template to render_django_template while we're at it? The former is deprecated since it's ambiguous in an XBlock context.

pomegranited commented 6 years ago

Sure thing @bradenmacdonald -- fixed with 87f384f

mtyaka commented 6 years ago

:+1: Thanks for updating the testing infrastructure!

pomegranited commented 6 years ago

Thanks @mtyaka ! I've re-added the "WIP" to the title so we don't forget to merge and tag https://github.com/edx/xblock-utils/pull/48 first.

pomegranited commented 6 years ago

Squashed a92a7be5ef0a38dd999ba991d5ac60dfbeaf9824...2bc51ca0235e91b38704670852f2d199cb2bdea4 into 59d7c6c9c260dfc61b67519e6c71e4c0dbb6da94 to make it cleaner to use this change on the client's fork.

naeem91 commented 6 years ago

Hi @pomegranited, should we wait for this to merge before bumping the version in requirements?

https://github.com/edx-solutions/edx-platform/blob/integration/requirements/edx/custom.txt#L6

pomegranited commented 6 years ago

Rebased 59d7c6c on master to resolve conflicts.

pomegranited commented 6 years ago

@naeem91 It's a bit confusing -- hard to manage this many open PRs and their dependencies!

When https://github.com/edx-solutions/edx-platform/pull/1032 merged to integration, it used temporary branches named edx-solutions:MCKIN-7023-needs-i18n to provide the i18n changes made in https://github.com/edx/xblock-utils/pull/48 and here.

Just now, I merged the latest master into edx-solutions:MCKIN-7023-needs-i18n, so that it includes the changes from this PR and https://github.com/edx-solutions/xblock-image-explorer/pull/38.

So to bump the version in integration's requirements.txt to include the changes for both these PRs now, you can use commit b7febc7a9a214cf4901c9cc864d4fdd56c4224d5.

CC @bradenmacdonald

naeem91 commented 6 years ago

@pomegranited sure. Thank you for taking care of this!

pomegranited commented 6 years ago

Redeployed the sandbox server, and ensured that template translations are showing as expected.

Merging now.

pomegranited commented 6 years ago

FYI @naeem91 -- I've submitted https://github.com/edx-solutions/edx-platform/pull/1073 to use this new version, so once that's merged we won't need to run off the temporary branch anymore.