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

Make DnDv2 themable #48

Merged
merged 10 commits into from
Jan 15, 2016
Merged

Make DnDv2 themable #48

merged 10 commits into from
Jan 15, 2016

Conversation

itsjeyd
Copy link
Contributor

@itsjeyd itsjeyd commented Jan 13, 2016

This PR adds the necessary infrastructure for applying custom themes to DnDv2. It also makes legacy (Apros) styling of the block available as an opt-in theme.

Screenshot

Appearance of a DnDv2 exercise with legacy theme enabled:

apros-theme

Testing

  1. Verify that DnDv2 uses default colors when legacy theme is not enabled.
  2. Follow instructions in updated README to enable legacy theme, then verify that DnDv2 uses styles shown in screenshot above.

@@ -123,7 +123,7 @@
</div>
<div class="row">
<label for="item-{{id}}-numerical-margin">
{{i18n "Margin ± (when a numerical value is required, values entered by students must not differ from the expected value by more than this margin; default is zero)"}}
{{i18n "Margin +/- (when a numerical value is required, values entered by students must not differ from the expected value by more than this margin; default is zero)"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? Personally I prefer the correct symbol, but don't have a strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradenmacdonald

After replacing load_resource with ResourceLoader.load_unicode in the line that pulls in js_templates.html for the EDIT dialog, I got the following error:

Error: 'ascii' codec can't decode byte 0xc2 in position 5557: ordinal not in range(128)

The difference between load_resource and load_unicode from xblock-utils is that the former wasn't calling unicode on the content pulled from a resource before returning it.

Changing the symbol seemed like a better option than keeping load_resource just for the sake of avoiding this error, but let me know -- maybe there's another way to approach this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@itsjeyd Hmm, you'd think a method called load_unicode would support loading unicode text :-p

Your workaround is fine for now, so we can get this merged on time. But the proper fix is to change xblock-utils to use return unicode(resource_content, 'utf-8') instead of just return unicode(resource_content). Would you be able to open a second PR to fix that in xblock-utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already mentioned this on OC-1164 but for reference, openedx-unsupported/xblock-utils#36 implements the fix for this issue.

Use the more specific "background-color" instead of "background" where applicable.
This was mainly done to improve the appearance of DnDv2 exercises in
Apros but the appearance of DnDv2 exercises in the LMS benefits from it
as well.
items to become visible before making assertions.
@bradenmacdonald
Copy link
Contributor

👍 Great work @itsjeyd ! Just tested in Apros with the strategy course and I'm really happy with how it looks.

Let's merge this now. I've made a note on the big PR, #34, that we can restore the "±" character if the xblock-utils fix makes it into Dogwood.

itsjeyd added a commit that referenced this pull request Jan 15, 2016
@itsjeyd itsjeyd merged commit 1a119b6 into openedx:master Jan 15, 2016
itsjeyd added a commit that referenced this pull request Jan 21, 2016
#48.

While working on getting the Drag and Drop v2 XBlock ready for edx.org,
we discovered a bug in xblock-utils that kept resources containing
non-ASCII chars from being loaded correctly. This bug was fixed in
openedx-unsupported/xblock-utils#36, and the fix was integrated
into edx-platform via
https://github.com/edx/edx-platform/pull/11282 (target branch: named-release/dogwood.rc) and
https://github.com/edx/edx-platform/pull/11283 (target branch: master).
@bradenmacdonald bradenmacdonald deleted the add-theming branch September 30, 2017 21:18
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.

2 participants