implement Moyo header #6107
implement Moyo header #6107
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While your code is mostly on the right track, there are a few critical issues that need to be addressed before we can approve the pull request:
-
CSS Issues:
- The CSS variable
--header-hightis misspelled. Please correct it to--header-heightto ensure consistency and functionality. - The
paddingproperty in the.headerclass is missing a unit (e.g.,px). This could lead to unexpected layout issues, so please specify the unit.
- The CSS variable
-
HTML Issues:
- The
hrefattributes in the navigation links are all set to'/', which redirects to the homepage. If these links are intended to navigate to specific sections or pages, please update thehrefattributes to point to the correct URLs.
- The
These changes are important to ensure that the navigation works as expected and that the layout is correctly implemented. Once these issues are resolved, we can proceed with approving your pull request. Keep up the great work, and don't hesitate to reach out if you need any assistance! 🚀
✨ 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.
src/index.html
Outdated
| <ul class="list"> | ||
| <li class="list-item"> | ||
| <a | ||
| href="/" |
There was a problem hiding this comment.
The href attribute is set to '/', which means this link will redirect to the homepage. If this is not the intended behavior, consider updating the href to point to the correct URL for the 'Apple' section.
src/index.html
Outdated
| </li> | ||
| <li class="list-item"> | ||
| <a | ||
| href="/" |
There was a problem hiding this comment.
The href attribute is set to '/', which means this link will redirect to the homepage. If this is not the intended behavior, consider updating the href to point to the correct URL for the 'Samsung' section.
src/index.html
Outdated
| </li> | ||
| <li class="list-item"> | ||
| <a | ||
| href="/" |
There was a problem hiding this comment.
The href attribute is set to '/', which means this link will redirect to the homepage. If this is not the intended behavior, consider updating the href to point to the correct URL for the 'Smartphones' section.
src/index.html
Outdated
| </li> | ||
| <li class="list-item"> | ||
| <a | ||
| href="/" |
There was a problem hiding this comment.
The href attribute is set to '/', which means this link will redirect to the homepage. If this is not the intended behavior, consider updating the href to point to the correct URL for the 'Laptops & Computers' section.
src/index.html
Outdated
| </li> | ||
| <li class="list-item"> | ||
| <a | ||
| href="/" |
There was a problem hiding this comment.
The href attribute is set to '/', which means this link will redirect to the homepage. If this is not the intended behavior, consider updating the href to point to the correct URL for the 'Gadgets' section.
src/index.html
Outdated
| </li> | ||
| <li class="list-item"> | ||
| <a | ||
| href="/" |
There was a problem hiding this comment.
The href attribute is set to '/', which means this link will redirect to the homepage. If this is not the intended behavior, consider updating the href to point to the correct URL for the 'Video' section.
src/style.css
Outdated
| font-family: Roboto, serif; | ||
| font-size: 12px; | ||
|
|
||
| --header-hight: 60px; |
There was a problem hiding this comment.
There is a typo in the CSS variable name --header-hight. It should be --header-height to correctly reflect its purpose.
src/style.css
Outdated
| .header { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| padding: 0 50; |
There was a problem hiding this comment.
The padding value 0 50 is missing a unit (e.g., px, em). This could lead to unexpected behavior. Consider specifying the unit for the padding value.
src/style.css
Outdated
| .link { | ||
| position: relative; | ||
| display: block; | ||
| height: var(--header-hight); |
There was a problem hiding this comment.
The CSS variable --header-hight is used here, but it is misspelled. Ensure it matches the corrected variable name --header-height.
src/style.css
Outdated
| position: relative; | ||
| display: block; | ||
| height: var(--header-hight); | ||
| line-height: var(--header-hight); |
There was a problem hiding this comment.
The CSS variable --header-hight is used here, but it is misspelled. Ensure it matches the corrected variable name --header-height.
There was a problem hiding this comment.
Great job on addressing the previous issues in your code! 🎉 Your updates to the HTML and CSS files have resolved the key problems, such as correcting the href attributes in the HTML and fixing the CSS variable spelling and padding units. Your code is now well-structured and meets the task requirements.
Remember, even though the decision is to approve, it's important to review the comments to further improve your skills. Keep an eye on details like spelling and unit usage in CSS, as these can impact the functionality and presentation of your work.
Keep up the good work and continue learning from each task! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
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 ❗️
The Google Fonts Configuration follows requirements.