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

New Homepage #255

Merged
merged 13 commits into from
Jun 30, 2017
Merged

New Homepage #255

merged 13 commits into from
Jun 30, 2017

Conversation

donjo
Copy link
Contributor

@donjo donjo commented Apr 14, 2017

👀 Preview on Federalist

The post-1.0 homepage to go out with 1.1.0

@shawnbot
Copy link
Contributor

I added a preview link to this.

@romkedehaan romkedehaan added this to the Cinco de Mayo Sprint milestone Apr 25, 2017
@shawnbot shawnbot changed the title WIP - New Homepage [WIP] New Homepage Apr 27, 2017
@romkedehaan romkedehaan requested a review from rtwell May 8, 2017 17:38
@donjo donjo removed this from the Pizza Party Day Sprint milestone May 23, 2017
@donjo donjo changed the title [WIP] New Homepage New Homepage Jun 29, 2017
@donjo donjo requested a review from toolness June 29, 2017 22:09
Copy link
Contributor

@toolness toolness left a comment

Choose a reason for hiding this comment

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

I've only looked at this on IE11 so far but I found one blocker (I think). I'll review the code separately, if that's okay.

On IE11, the GSA logo at the bottom of the page looks tiny:

tiny_gsa_logo

Also, the diamond in the middle of the horizontal rule-type-thing is smooshed in IE11:

smooshed_diamond

The latter might be okay, since it's a decorative flourish that might be acceptable to degrade on IE11, but the former seems like a blocker...

}

// Override for 4x1 media grid when it collapses to 2x2
@media screen and (max-width: 1200px) and (min-width: 600px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, there aren't any SASS variables we can reuse instead of hard-coding 1200px and 600px here? Just curious, I'm not very familiar w/ the USWDS SASS/CSS yet.

@toolness toolness dismissed their stale review June 30, 2017 16:48

Oops, didn't realize that the footer image wasn't actually part of this PR!

@toolness
Copy link
Contributor

Um, I don't seem to have the ability to re-review a PR once I've dismissed it, so I guess I'll just leave a comment here...

I looked at the code and it looks good! I say 🚢 if you are OK with the smooshed diamond on IE11. I've filed the footer bug separately as #331.

@donjo donjo removed the request for review from rtwell June 30, 2017 17:43
@donjo donjo requested a review from toolness June 30, 2017 17:43
Copy link
Contributor

@toolness toolness left a comment

Choose a reason for hiding this comment

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

Ah there we go! Woot!

@toolness toolness merged commit 8747ba8 into develop Jun 30, 2017
@toolness toolness deleted the new-homepage branch June 30, 2017 17:51
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

4 participants