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

Clean up front page UI and video elements, remove cruft [OPEN-407, OPEN-358] #6243

Merged
merged 10 commits into from
Jan 7, 2015
Merged

Clean up front page UI and video elements, remove cruft [OPEN-407, OPEN-358] #6243

merged 10 commits into from
Jan 7, 2015

Conversation

sarina
Copy link
Contributor

@sarina sarina commented Dec 14, 2014

Ref: https://openedx.atlassian.net/secure/attachment/14110/default-view-audit.pdf, https://openedx.atlassian.net/browse/OPEN-407

  • Change "Terms of Service & Honor Code" to conditionally link to the "honor code" page if the site defines a link to the honor code in the marketing url map.
  • Remove reference to old marketing video
  • Strike Jobs, News pages
  • Change "Vision" header on default About page to "About"
  • Change bright blue default background (index page) to something... better
  • Change very light grey default background (course detail page, find course logged in view) to something better (same color as above, probably)
  • Remove any cruft encountered:
    • lms/templates/contact.html
    • lms/templates/edx-legacy.html
  • Change "EXPLORE FREE COURSES FROM LEADING UNIVERSITIES." text on the "Find Courses" page

sarina
sarina reviewed Dec 14, 2014
View reviewed changes
lms/templates/index.html
@@ -26,10 +27,12 @@
% else:
% if self.stanford_theme_enabled():
<h1>${_("Free courses from <strong>{university_name}</strong>").format(university_name="Stanford")}</h1>
<h2>${_("For anyone, anywhere, anytime")}</h2>
% else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this change is to remove our stale, early-2012 marketing text from default Open edX instances. Cribbed some text from the apache welcome page and the django welcome page - I'm open to suggestions here.

@sarina sarina changed the title Clean up front page UI and video elements [OPEN-407] Clean up front page UI and video elements, remove cruft [OPEN-407, OPEN-358] Dec 14, 2014
@stephensanchez
Copy link
Contributor

@sarina One quick note (and I may be out of context) I noticed that the wording suggests removing "Honor Code". Maybe this is edx.org specific, but the Honor code is part of the Terms of Service page
https://www.edx.org/edx-terms-service

@sarina sarina force-pushed the sarina/default-edx-UI branch 3 times, most recently from e44cd91 to da22aec Compare January 5, 2015 20:50
@sarina
Copy link
Contributor Author

sarina commented Jan 5, 2015

@singingwolfboy @talbs can you please review?

singingwolfboy
singingwolfboy reviewed Jan 5, 2015
View reviewed changes
lms/templates/footer.html
<a href="${marketing_link('TOS')}">${_("Terms of Service and Honor Code")}</a>
<a href="${marketing_link('TOS')}">${_("Terms of Service")}</a>
%if marketing_link('HONOR') and marketing_link('HONOR') != '#':
and <a href="${marketing_link('HONOR')}">${_("Honor Code")}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now, "and" isn't being translated. Is that OK?

@sarina sarina force-pushed the sarina/default-edx-UI branch from da22aec to e13a722 Compare January 6, 2015 03:32
sarina
sarina reviewed Jan 6, 2015
View reviewed changes
lms/templates/footer.html
honor_link = u"<a href='{}'>".format(marketing_link('HONOR'))
%>
${
_("{tos_link_start}Terms of Service{tos_link_end} and {honor_link_start}Honor Code{honor_link_end}").format(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@singingwolfboy how's this?

I am having trouble, however, figuring out how to verify if this logic is even working when there is no honor code link present. Noting it down so I remember to look into it tomorrow - I thought i verified the similar logic at L17 and 24 above previously.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - was able to verify this. Had to actually fully restart my server to see the %if clause take effect.

@sarina sarina force-pushed the sarina/default-edx-UI branch 2 times, most recently from 775a287 to 141a71e Compare January 6, 2015 18:45
@sarina
Copy link
Contributor Author

sarina commented Jan 6, 2015

@stephensanchez thanks for pointing out the honor code. I made the open source footer conditionally link to the Honor Code if there's a link present to an Honor Code in the marketing urls map.

@sarina
Copy link
Contributor Author

sarina commented Jan 6, 2015

@singingwolfboy any other review comments?

@nedbat could you please also review this?

@singingwolfboy
Copy link
Contributor

👍

@talbs
Copy link
Contributor

talbs commented Jan 6, 2015

@sarina, apologies for the delay - I'll be getting to this shortly on my end.

talbs
talbs reviewed Jan 6, 2015
View reviewed changes
lms/templates/index.html
@@ -26,10 +27,13 @@
% else:
% if self.stanford_theme_enabled():
<h1>${_("Free courses from <strong>{university_name}</strong>").format(university_name="Stanford")}</h1>
<h2>${_("For anyone, anywhere, anytime")}</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: indentation - looks like you might be missing some spaces.

@talbs
Copy link
Contributor

talbs commented Jan 6, 2015

@sarina, the HTML and Sass/CSS changes here look good. Thanks much for helping to clean up our Open front door.

👍

nedbat
nedbat reviewed Jan 6, 2015
View reviewed changes
lms/templates/footer.html
${_("News")}
</a>
</li>
%if marketing_link('JOBS') and marketing_link('JOBS') != '#':
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems mis-indented.

@nedbat
Copy link
Contributor

nedbat commented Jan 6, 2015

👍 after fixes.

@sarina sarina force-pushed the sarina/default-edx-UI branch from d7c596d to c83030c Compare January 6, 2015 22:19
@sarina sarina force-pushed the sarina/default-edx-UI branch from c83030c to 414e050 Compare January 6, 2015 22:24
@sarina sarina force-pushed the sarina/default-edx-UI branch from 414e050 to 4fcbcaa Compare January 6, 2015 23:27
@sarina
Copy link
Contributor Author

sarina commented Jan 7, 2015

last build had Jenkins system failures in a few shards, so rebuilding.

@sarina
Copy link
Contributor Author

sarina commented Jan 7, 2015

failing test is flaky

sarina added a commit that referenced this pull request Jan 7, 2015
@sarina
Merge pull request #6243 from edx/sarina/default-edx-UI
8af5494 
Clean up front page UI and video elements, remove cruft [OPEN-407, OPEN-358]
@sarina sarina merged commit 8af5494 into master Jan 7, 2015
@sarina sarina deleted the sarina/default-edx-UI branch January 7, 2015 02:58
@snyk-bot snyk-bot mentioned this pull request Jul 16, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sarina @stephensanchez @singingwolfboy @talbs @nedbat