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

Progress: adjust height and align header elements #21988

Merged
merged 1 commit into from Apr 24, 2018

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Apr 20, 2018

Before:
screen shot 2018-04-20 at 3 59 45 pm

After:
screen shot 2018-04-20 at 3 59 28 pm

@@ -40,6 +39,7 @@ class LessonSelector extends Component {
<option
value={lesson.position}
key={lesson.id}
style={{padding: 10}}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move to style component instead of inline

Copy link
Contributor

@caleybrock caleybrock left a comment

Choose a reason for hiding this comment

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

have you checked this on IE, safari, firefox?

@@ -8,8 +8,7 @@ const styles = {
display: 'block',
boxSizing: 'border-box',
fontSize: 'medium',
padding: '0.8em',
marginBottom: 10,
height: 34
},
heading: {
marginBottom: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why is there a comma after the marginBottom (it's the only property:value pair for heading)? There isn't a comma after (height: 34), which is inline with the style guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, the comma is on line 15 is probably because I had a style after marginBottom that I ended up not needing so I removed it. I have a slight preference for trailing commas because if I add something later, I will only see the change that I added in the diff not the changes that I added AND the additional comma on the line preceding the change. 🤷🏼‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

Copy link
Contributor

@nkiruka nkiruka left a comment

Choose a reason for hiding this comment

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

LGTM!

@Erin007 Erin007 merged commit 99932bc into staging Apr 24, 2018
@Erin007
Copy link
Contributor Author

Erin007 commented Apr 24, 2018

Since this is still behind a flag, I'll spot check the other browsers on staging.

@Erin007 Erin007 deleted the progress-dropdown-styles branch April 24, 2018 00:12
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