London class-8-Helena-Rog-HTML/CSS-week-2#311
London class-8-Helena-Rog-HTML/CSS-week-2#311hachi-ops wants to merge 22 commits intoCodeYourFuture:masterfrom
Conversation
added classes to index.html applied more flexbox to new classes
flex changes button positioning and styling fixed colors/typography added hover effect
comments added button color changed color changes to the background img
added section: image markup styling
jstadnik
left a comment
There was a problem hiding this comment.
Easy to read code, with just enough classes to make them meanigful/useful, but at the same time easily referenceable -- a pleasure to review :)
Great work on meeting your deadlines ahead of time :)
css/style.css
Outdated
| padding: 0; | ||
| box-sizing: border-box; | ||
| width: 100%; | ||
| font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
The font in your page is showing up as sans-serif for me -- I think this may be due to the duplicated font-family property in this section. Notice how this one (which most likely overwrites the top one, as css is ranked from bottom to top) does not put Roboto in quotes -- maybe that's why?
index.html
Outdated
| @@ -1,3 +1,7 @@ | |||
| <!-- Add your HTML markup here --> | |||
There was a problem hiding this comment.
No need to leave these comments behind! In fact, all non-relevant comments should not make it to the final PR.
index.html
Outdated
| <li><a href="#">Blog</a></li> | ||
| <li><a href="#">Login</a></li> | ||
| </ul> | ||
| <img id="hamburger" src="img/menu-hamburger.svg" alt=""/> |
There was a problem hiding this comment.
I was not able to make this image appear?
index.html
Outdated
| </ul> | ||
| <img id="hamburger" src="img/menu-hamburger.svg" alt=""/> | ||
| </nav> | ||
| <header class="hero"> |
There was a problem hiding this comment.
The header is the part at the very top -- the logo and navigation. By that definition, hero should never find itself in the header, really. It should be a normal section. But it is indeed the hero :)
css/style.css
Outdated
| } | ||
|
|
||
| #hamburger { | ||
| display: none; |
There was a problem hiding this comment.
Things with display none should not be left behind, unless the display settings are toggled in some way to make them appear at least sometimes.
|
|
||
| <main> | ||
| <section> | ||
| <div> |
There was a problem hiding this comment.
Do you actually need a div here? I cannot see it being targeted by any css rules.
index.html
Outdated
| </div> | ||
| <div class="section-items"> | ||
| <div class="section-item"> | ||
| <img src="img/icon-devices.svg" alt="" /> |
There was a problem hiding this comment.
Images should have an alt text -- otherwise someone accessing this on a screen reader would not be able to tell what they are. For example "phone, computer and tablet".
css/style.css
Outdated
|
|
||
| /* Section */ | ||
|
|
||
| .section-items { |
css/style.css
Outdated
| } | ||
|
|
||
| .section-item img { | ||
| width: 160px; |
There was a problem hiding this comment.
Should not be pixels -- use relative measurements where possible (need to be careful if they're not svg files, in case the relative measurement starts to be bigger than the actual image size, resulting in blurring)
index.html
Outdated
| <hr/> | ||
| <p class="first-p">Join us on</p> | ||
| <div class="footer-icons"> | ||
| <img src="img/twitter-icon.svg" alt="twitter" width="20"/> |
There was a problem hiding this comment.
As discussed in class, these should be wrapped in a div to prevent the borded squashing the image.
Store page and new karma section
|
Helena, I think your Karma page looks great! I liked how you used semantic tags. And I liked the clean structure. Well done! |
Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in
HOW_TO_MARK.mdin the root of this repositoryYour Details
Homework Details
Notes
Layout/hover
Matching colours/paddings/font size/weight
My way around github
That's it