add task solution #5354
add task solution #5354
Conversation
There was a problem hiding this comment.
Great job on your Pull Request! Just a few minor points to consider for future work: make sure to include fallback fonts, avoid using the * selector for better performance, and be careful with the syntax in anchor href attributes (no spaces and start with #). Also, enhance your alt text for images to be more descriptive, and keep an eye on indentation and spacing for better readability. Keep up the good work! 👍💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <header> | ||
| <div class="logo-container"> | ||
| <a href="#"> | ||
| <img | ||
| class="logo-img" | ||
| src="./images/logo.png" | ||
| alt="logo" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
Remember about correct indentation between parent and child elements. Each level of nesting, including text, contained inside the element, requires 2-space offset.
| <nav class="header-nav"> | ||
| <ul class="header-nav-list"> | ||
| <li class="header-nav-item"> | ||
| <a | ||
| href="#" | ||
| class="header-nav-link is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li class="header-nav-item"> | ||
| <a | ||
| href="#" | ||
| class="header-nav-link" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="header-nav-item"> | ||
| <a | ||
| href="#" | ||
| class="header-nav-link" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li class="header-nav-item"> | ||
| <a | ||
| href="#" | ||
| data-qa="hover" | ||
| class="header-nav-link" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="header-nav-item"> | ||
| <a | ||
| href="#" | ||
| class="header-nav-link" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li class="header-nav-item"> | ||
| <a | ||
| href="#" | ||
| class="header-nav-link" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li class="header-nav-item"> | ||
| <a | ||
| href="#" | ||
| class="header-nav-link" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li class="header-nav-item"> | ||
| <a | ||
| href="#" | ||
| class="header-nav-link" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> | ||
| </ul> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML to add some "air" and simplify reading.
| <img | ||
| class="logo-img" | ||
| src="./images/logo.png" | ||
| alt="logo" |
There was a problem hiding this comment.
The alt attribute should describe the image content. Instead of 'logo', it should be more descriptive regarding what the logo represents or the company name.
| <h1>Moyo header</h1> | ||
| <header> | ||
| <div class="logo-container"> | ||
| <a href="#"> |
There was a problem hiding this comment.
Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol followed by the link target. For example, href="#home" if it's linking to an element with id="home".
| <li class="header-nav-item"> | ||
| <a | ||
| href="#" | ||
| class="header-nav-link is-active" |
There was a problem hiding this comment.
Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol followed by the link target.
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Don't use * selector (it impacts performance). Set styles only for elements that require them. Zeroing out your margins, paddings or other styles with '*' is still inefficient for browser.
| align-items: center; | ||
| display: flex; | ||
| height: 60px; | ||
| font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
Remember to use fallback fonts - alternative font-family in case the main one doesn't work. For example, you could use font-family: Roboto, Arial, sans-serif;.
DEMO LINK
TEST REPORT LINK
Header height is set in 1 place (for the links)
Content is vertically centered (for any header height)
CSS is used to show all letters in Uppercase (don't type them in HTML)
Logo is an image wrapped with a link
CSS Variable is used for a blue color
Pseudo-element is used for a blue line below the active link
Code follows all the Code Style Rules ❗️