Task solved #5900
Task solved #5900
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 🌟 While your code is mostly compliant with the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
CSS Selector Usage: The use of the
gapproperty in the.nav__listclass is not supported according to the task requirements. Please replace it with margins for indents. -
Header Width: The header should stretch the full page width without using horizontal margins. Currently, the
.headerclass usespadding: 0 50px;, which does not align with this requirement. -
Logo Placement: Ensure that the logo is not part of the
<nav>element as specified in the task requirements.
These issues are critical as they affect the core requirements of the task. Once these are addressed, your code will be in great shape! Keep up the great work, and don't hesitate to reach out if you need any help. 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| </head> | ||
| <body> | ||
| <h1>Moyo header</h1> | ||
| <header class="header"> |
There was a problem hiding this comment.
Consider using a <nav> element instead of a <header> for the navigation section to improve semantic structure. This change can enhance accessibility by clearly indicating the navigation role to assistive technologies.
| href="index.html" | ||
| class="logo__link" | ||
| > | ||
| <img |
There was a problem hiding this comment.
Ensure that the image path 'images/logo.png' is correct and that the image exists in the specified location. This will prevent broken image links on the webpage.
| alt="Moyo logo" | ||
| /> | ||
| </a> | ||
| <nav class="nav"> |
There was a problem hiding this comment.
Consider adding aria-label or aria-labelledby attributes to the <nav> element to provide a descriptive label for screen readers, enhancing accessibility.
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a |
There was a problem hiding this comment.
The 'is-active' class is applied to the 'Apple' link. Ensure this class is dynamically managed if this is meant to indicate the current page, to avoid misleading users.
src/style.css
Outdated
| --link-color: #000; | ||
| --active-link-color: #00acdc; |
There was a problem hiding this comment.
Consider using more descriptive variable names for colors, such as --primary-link-color and --highlight-link-color, to make the purpose of each variable clearer.
| position: relative; | ||
| height: 60px; | ||
| text-transform: uppercase; | ||
| font-family: Roboto, serif; |
There was a problem hiding this comment.
The font stack includes 'Roboto' and 'serif'. Ensure that 'Roboto' is correctly loaded in your HTML file, as it is a sans-serif font, and consider using a fallback sans-serif font instead of 'serif'.
src/style.css
Outdated
| .is-active { | ||
| color: var(--active-link-color); | ||
| } |
There was a problem hiding this comment.
The '.is-active' class is used to style active links. Ensure this class is applied dynamically to the correct element to reflect the current active page or section.
There was a problem hiding this comment.
Great job on addressing most of the previous issues! 🎉 Your code now meets the task requirements, with the header stretching the full page width and the logo correctly placed outside the <nav> element. However, there are still a few points to consider:
- Horizontal Margins: The use of
margin-right: 50px;in.navandmargin-left: 50px;in.logo__linkcontradicts the requirement for the header to stretch the full page width. Consider removing these margins to fully comply with the task requirements.
Moving forward, make sure to review these comments to improve your code. Addressing these critical issues will help you avoid similar problems in the future. Keep up the good work and continue learning from your experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <header class="header"> | ||
| <a | ||
| href="index.html" | ||
| class="logo__link" | ||
| > | ||
| <img | ||
| class="logo__img" | ||
| src="images/logo.png" | ||
| alt="Moyo logo" | ||
| /> | ||
| </a> | ||
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link is-active" | ||
| href="#" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| </nav> | ||
| </header> |
There was a problem hiding this comment.
Ensure that the header stretches the full page width without using horizontal margins. This is a critical requirement according to the task description.
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link is-active" | ||
| href="#" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| </nav> |
There was a problem hiding this comment.
Make sure that the logo is not part of the <nav> element. The current structure seems correct, but double-check to ensure compliance with the task requirements.
| } | ||
|
|
||
| .logo__link { | ||
| margin-left: 50px; |
There was a problem hiding this comment.
The use of margin-left: 50px; in .logo__link contradicts the requirement for the header to stretch the full page width without using horizontal margins. Consider removing this margin.
| } | ||
|
|
||
| .nav { | ||
| margin-right: 50px; |
There was a problem hiding this comment.
The use of margin-right: 50px; in .nav contradicts the requirement for the header to stretch the full page width without using horizontal margins. Consider removing this margin.
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 ❗️