overhangio / tutor-indigo

An elegant, customizable theme for Open edX
GNU Affero General Public License v3.0
74 stars 276 forks source link

Social media links contain broken text #11

Closed mrtndwrd closed 3 years ago

mrtndwrd commented 3 years ago

I'm not sure if this is an issue with this theme specifically, but the social media buttons on the right side of a course's "About" page show some weird behavior. This is what happens when I click the Twitter button on https://demo.openedx.overhang.io/courses/course-v1:edX+DemoX+Demo_Course/about

image

I've been trying to find the place where this message originates, but it's a bit weird. I only found it in the edx-platform repository at ./themes/stanford-style/lms/templates/courseware/course_about_sidebar_header.html, but that file doesn't include the Facebook button that I also see in the indigo theme, so I guess that that's not the sidebar template that's actually used by the indigo theme.

On a side note: it's not super clear to me what this theme depends on. In other words: if I don't override a file in the theme, which file is used? If you can help me answer this question I'll gladly open this issue in the parent theme instead of this repository :)

regisb commented 3 years ago

I think what you are looking for is the lms/templates/courseware/course_about_sidebar_header.html template :) Found by grepping "intent/tweet" in edx-platform. I am pretty sure we should replace:

tweet_action = u"http://twitter.com/intent/tweet?text={tweet_text}".format(tweet_text=six.moves.urllib.parse.quote_plus(tweet_text.encode('UTF-8')))

by:

tweet_action = "http://twitter.com/intent/tweet?text={tweet_text}".format(tweet_text=urllib.parse.quote(tweet_text))

(note that we replaced quote_plus by quote).

If you make an upstream PR to edx-platform, we can cherry-pick the commit in Tutor.

mrtndwrd commented 3 years ago

Thanks! I just found that template as well. I'll try out your suggestion and hope that edX responds to my MR when I make it :crossed_fingers:

regisb commented 3 years ago

Even if your PR does not get merged immediately, we can cherry-pick your commit. Just open a PR on the Tutor repo and add a line here: https://github.com/overhangio/tutor/blob/master/tutor/templates/build/openedx/Dockerfile#L63 (and a line in the changelog of course).

mrtndwrd commented 3 years ago

https://github.com/edx/edx-platform/pull/26393

mrtndwrd commented 3 years ago

Fixed upstream, so I'll close this. Thanks for your help!

regisb commented 3 years ago

Would you like to include the fix in the default openedx Docker image that ships with Tutor? If yes, you should add a curl https://github.com/edx/edx-platform/commit/....patch | git apply - statement to the openedx/Dockerfile. Would you like to open a PR?

mrtndwrd commented 3 years ago

To be honest I think it's not such an important fix that it needs to be hotfixed in the current release. I'm OK with waiting for the next major release. But if you would really like me to make this PR I'll do it. Let me know.