-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
ECOM-1339 Branding API footer #8175
Conversation
@@ -1827,7 +1872,7 @@ | |||
# Translators: This is the website name of www.facebook.com. Please | |||
# translate this the way that Facebook advertises in your language. | |||
"title": _("Facebook"), | |||
"icon": "fa-facebook-square" | |||
"icon": "fa-facebook" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marketing want this to be fa-facebook-square
Some minor changes but once updated and tests pass 👍 |
# Please do not translate any of these trademarks and company names. | ||
return _( | ||
u"\u00A9 {org_name}. All rights reserved except where noted. " | ||
u"EdX, Open edX and the edX and OpenEdX logos are registered trademarks " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Open edX" but not "OpenEdX" ?
de72ce0
to
1c52da4
Compare
@jimabramson I think I've addressed all your comments. Could you please take another look at this PR? @feanil confirmed yesterday that we aren't automatically clearing the default cache in deploys and recommended a 30 minute server-side cache timeout. |
@@ -427,6 +427,8 @@ | |||
# Student Notes | |||
url(r'^courses/{}/edxnotes'.format(settings.COURSE_ID_PATTERN), | |||
include('edxnotes.urls'), name="edxnotes_endpoints"), | |||
|
|||
url(r'^api/v1/branding/', include('branding.api_urls')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that this URL is not consistent with our API conventions. It should be /api/branding/v1/
(version at the end).
@AlasdairSwan I'm going to change this URL, so we'll need to update the URL on Drupal as well. Can't believe I didn't notice this until just now :\
Serve branded footer JSON/HTML/CSS/JS from an API endpoint in the branding app. Refactor OpenEdX and EdX.org footer templates to use the Python version of the API, ensuring that the API values are consistent with the footer included in main.html. Detailed changes: * Added footer API end-point to the branding app. * Footer API allows the language to be set with querystring parameters. * Footer API allows showing/hiding of the OpenEdX logo using querystring parameters. * Deprecate ENABLE_FOOTER_V3 in favor of the branding API configuration flag. * Move no referrer script into main.html from the edx footer template. * Rename rwd_header_footer.js to rwd_header.js * Cache API responses. Authors: Awais Qureshi, Aamir Khan, Will Daly
1c52da4
to
6af5fc1
Compare
👍 looks great @wedaly. (also, sorry about the incorrect remark regarding cache invalidation) |
Serve branded footer JSON/HTML/CSS/JS from an API endpoint
in the branding app. Refactor OpenEdX and EdX.org footer templates
to use the Python version of the API, ensuring that the API
values are consistent with the footer included in main.html.
Detailed changes:
Authors: Awais Qureshi, Aamir Khan, Will Daly
JIRA: ECOM-1339
@jimabramson @AlasdairSwan please review.
FYI: @awais786 @aamir-khan @singingwolfboy @feanil