Conversation
JustineZwiazek
left a comment
JustineZwiazek
left a comment
There was a problem hiding this comment.
Really good job Jenny. I would have loved to be able to see the images (not loading, I think the files are missing in the commit) to be able to review styling in more depth. Other than that I would say some small changes needed with paragraph and heading elements, and it could have more consistent code indentations to make it easier to read.
| <header class="main-header"> | ||
|
|
||
| <div class="logo"> | ||
| <img src="atletilogo.png" width="200" height="200" alt="AtletiLOGO"> |
There was a problem hiding this comment.
Really good to see that you added alt text. I would say that the format could be improved by separating the words by a space to make it more descriptive.
| <div class="logo"> | ||
| <img src="atletilogo.png" width="200" height="200" alt="AtletiLOGO"> | ||
|
|
||
| <div class="sitename"> |
There was a problem hiding this comment.
This is very minor, but I think best practice is separating words with a hyphen when naming classes, to make it easier to read.
|
|
||
| <div class="sitename"> | ||
| <p class="h1">Atleti.</p> | ||
| <p class="h2"><em>The number one source for the latest tennis news</em></p> |
There was a problem hiding this comment.
Suggestion here, I don't think you need to wrap the h1 and h2 elements in a paragraph element. You could instead do a <h1 class="...">. Or with no class if not needed.
|
|
||
| </header> | ||
| <article class="main-article"> | ||
| <img class="article-pic" src="novak.jpeg" alt="Novak Djokivic" style="width:100%"> |
There was a problem hiding this comment.
Really nice job with naming classes here (also big fan of Djokovic! :)
| <article class="main-article"> | ||
| <img class="article-pic" src="novak.jpeg" alt="Novak Djokivic" style="width:100%"> | ||
| <div class="djokovic"> | ||
| <p class="heading">AUSTRALIAN OPEN - Missing Novak Djokovic after historical vaccination mess. What's next for world tennis super star?</p> |
There was a problem hiding this comment.
Could be a good practice to replace the p element with a h element if it's a heading.
| } | ||
|
|
||
| .sitename{ | ||
| text-align:center; |
There was a problem hiding this comment.
Very small thing but correct indentations would make it easier to read.
| border-radius: 2%; | ||
| } | ||
|
|
||
| li a:hover { |
There was a problem hiding this comment.
I really liked the effect on the navigation bar.
| background-color: #F4E139; | ||
| } | ||
|
|
||
| @media (min-width: 667px) and (max-width: 1024px){ |
There was a problem hiding this comment.
I think here you are missing the transition from having a logo and the name of the page (desktop) to having just the logo (tablet) - see sketch of the assignment.
There was a problem hiding this comment.
I am reading the project specs again now and perhaps I am wrong here and this wasn't a fixed requirement?
2 participants
No description provided.