New solution #5387
New solution #5387
Conversation
There was a problem hiding this comment.
Great work on your Pull Request! 👍 Just a few minor points to keep in mind for future projects: Consider removing redundant CSS properties, fixing any syntax errors, adding fallback font-families, using descriptive alt text for images, and maintaining consistent formatting and indentation in your code. These small improvements will make your code even better! Keep it up! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <header class="header"> | ||
| <a | ||
| href="#" | ||
| class="header__link" | ||
| > | ||
| <img | ||
| class="header__logo" | ||
| src="./images/logo.png" | ||
| alt="logo" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
Remember about correct indentation between parent and child elements. The <a> tag should be indented by 2 spaces inside the <header> tag, and its child <img> tag should be indented by 2 spaces inside the <a> tag.
| <link | ||
| rel="stylesheet" | ||
| href="./style.css" | ||
| /> | ||
| </head> | ||
| <body> | ||
| <h1>Moyo header</h1> | ||
| <header class="header"> | ||
| <a |
There was a problem hiding this comment.
Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol and should not have spaces.
| <a | ||
| href="#" | ||
| class="header__link" | ||
| > | ||
| <img | ||
| class="header__logo" | ||
| src="./images/logo.png" | ||
| alt="logo" | ||
| /> |
There was a problem hiding this comment.
Keep your attributes correctly formatted. If the HTML element has long attribute values or more than 2 attributes, start each one, including the first, on a new line with 2-space indentation related to the tag.
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| </nav> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML to add some 'air' and simplify reading. But don't add them between parent and child elements.
| > | ||
| <img | ||
| class="header__logo" | ||
| src="./images/logo.png" |
There was a problem hiding this comment.
The alt attribute should describe the image content. Instead of 'logo', a more descriptive text could be used.
| @@ -1,3 +1,79 @@ | |||
| body { | |||
| margin: 0; | |||
| font-family: Roboto, sans-serif; | |||
There was a problem hiding this comment.
It's good practice to specify a fallback font-family, in case the primary font cannot be loaded. Consider adding a generic font family as a fallback.
| justify-content: space-between; | ||
| align-items: center; | ||
| padding: 0 50px; | ||
| box-shadow: 0 2px 4 px rgba(0 0 0 0.05); |
There was a problem hiding this comment.
There is a syntax error in the box-shadow property value. The color function rgba should have commas separating the values.
| } | ||
|
|
||
| .nav { | ||
| display: block; |
There was a problem hiding this comment.
The .nav class has a display: block; property, which is the default display value for the nav element. This line can be removed as it is redundant.
| </li> | ||
| <li class="nav__item"> |
There was a problem hiding this comment.
Add the blank line between the all links
| </li> | |
| <li class="nav__item"> | |
| </li> | |
| <li class="nav__item"> |
| @@ -1,3 +1,79 @@ | |||
| body { | |||
| margin: 0; | |||
| font-family: Roboto, sans-serif; | |||
There was a problem hiding this comment.
Move font styles to the html selector
| } | ||
|
|
||
| .is-active { | ||
| color: #00acdc; |
There was a problem hiding this comment.
Create a variable for repeated color and use it everywhere
Co-authored-by: Volodymyr Soltys <64750958+volodymyr-soltys97@users.noreply.github.com>
DEMO LINK
TEST REPORT LINK
[ x ] Header height is set in 1 place (for the links)
[ x ] Content is vertically centered (for any header height)
[ x ] CSS is used to show all letters in Uppercase (don't type them in HTML)
[ x ] Logo is an image wrapped with a link
[ x ] CSS Variable is used for a blue color
[ x ] Pseudo-element is used for a blue line below the active link
[ x ] Code follows all the Code Style Rules ❗️