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

Home: aesthetic improvements #16611

Merged
merged 3 commits into from Jul 26, 2017
Merged

Home: aesthetic improvements #16611

merged 3 commits into from Jul 26, 2017

Conversation

breville
Copy link
Member

@breville breville commented Jul 25, 2017

without sections:

home aesthetic nosections

with sections:

home aesthetic sections

@@ -139,14 +143,18 @@ const Notification = React.createClass({
bullhorn: 'bullhorn'
};

const buttonStyle = type !== "course" ? styles.button : [styles.button, styles.courseButton];
Copy link
Contributor

Choose a reason for hiding this comment

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

I always prefer ternaries to use the positive when possible:

 const buttonStyle = type === "course" ? [styles.button, styles.courseButton] : styles.button;

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think we could/should use NotificationType.course instead of course here and elsewhere in this file.

@@ -46,6 +45,10 @@ const styles = {
borderWidth: 1,
borderColor: 'red'
},
actionBox: {
float: 'right',
marginRight: 21
Copy link
Contributor

Choose a reason for hiding this comment

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

21 seems pretty specific. Worth a comment on how we got to this number?

/>
<div>
<Notification
type="course"
Copy link
Contributor

Choose a reason for hiding this comment

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

NotificationType.course

@tanyaparker
Copy link
Contributor

  • Can we also change the string to lowercase "Join a section"
  • Can we make the text in the wide course card the same size as the smaller course cards?
  • What do you think about making the description text of Classroom Sections also the same smaller text? I think it clutters the hierarchy a little bit. I like keeping most of the text on the page the same and only giving more weight to the column headers in the sections table and the headings like "My Courses" and "Find a course." And for consistency I think that making this description text style smaller should apply to all similar components like the Hour of Code component you made on the Courses page (which we can do later if it doesn't fit in the context of this PR).

@breville breville merged commit c1f9f4b into staging Jul 26, 2017
@breville breville deleted the home-aesthetic-improvements branch July 26, 2017 01:09
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.

None yet

3 participants