Conversation
Galaxylaughing
left a comment
Galaxylaughing
left a comment
There was a problem hiding this comment.
I'm not sure what to say here, but I finished my review. Cool site :)
|
|
||
|
|
||
| <section> | ||
| <section id="reasons"> |
There was a problem hiding this comment.
I like your class names. reasons is a lot more straightforward and flexible than mine, which was like why_us or something.
|
|
||
|
|
||
| <section> | ||
| <section id='footer'> |
There was a problem hiding this comment.
Maybe it's just my interpretation of the instructions, but I think you could have changed the <section> tag around this section to <footer>, which would have respected semantic HTML more than the original and negated the need for an id for this section.
| @@ -0,0 +1,127 @@ | |||
| @import url('https://fonts.googleapis.com/css?family=Open+Sans'); | |||
There was a problem hiding this comment.
Good use of @import. I won't even say how long it took me to figure this bit of syntax out, because it was absolute ages lol
| @import url('https://fonts.googleapis.com/css?family=Open+Sans'); | ||
|
|
||
| html { | ||
| font-family: 'Open Sans', sans-serif; |
There was a problem hiding this comment.
I like that you added sans-serif here as a secondary option, so that if something goes wrong with the @import API call (or whatever it is, exactly), there will be a back-up font choice. I didn't think to do this in mine.
| } | ||
|
|
||
| section:hover + section { | ||
| background: #e5e5e5; |
There was a problem hiding this comment.
I like the idea here, I think it's a really cool way to focus attention on the section the user is currently focused on. However, I think the the user experience was confusing because of the fact that the nav and splash are not included in this process, even though the other sections and footer (because the footer is a section) were included.
It could have made a clearer user experience, at least for me, if you had included the nav and splash so that they also turned gray when you weren't hovering over them.
| background: #e5e5e5; | ||
| } | ||
|
|
||
| ul { |
There was a problem hiding this comment.
I notice that, when you shrink the browser width down, the About line gets wrapped such that it appears under Startrly, and when you shrink it more, Try Now! and About both wrap such that Try Now! appears under Startrly but About appears adjacent to Try Now! and on the same line. I'm not totally sure why, but I assume it has to do with the flexbox rules about wrapping. Maybe if you turned off wrap, they would remain on the same line and instead the space between Startrly and Try Now! would shrink?
|
|
||
| #banner a { | ||
| width: 10em; | ||
| height: 2em; |
There was a problem hiding this comment.
I like that you set the height of the 'call to action'. I used padding to add space between the words and the border, but I think yours is a better solution.
|
|
||
| margin: 2em; | ||
| text-align: left; | ||
| font-size: .66em; |
There was a problem hiding this comment.
I think this font size is really hard to read.
| border-top: 1px solid lightgrey; | ||
| } | ||
|
|
||
| #footer>div { |
There was a problem hiding this comment.
I'm not sure precisely which line is controlling this, but I like that the "Support" and "About" headers are off-set from their respective lists. It looks cool.
There was a problem hiding this comment.
I spent a long time trying to make that not happen :)
2 participants
Startrly
Congratulations! You're submitting your assignment.
Comprehension Questions