MOJO header #6114
MOJO header #6114
Conversation
ivsasha
commented
Feb 11, 2025
ivsasha
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While you've made great progress, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Use of
*Selector: The use of the universal selector*for resetting styles can impact performance and is against the task requirements. Consider using a more targeted approach for resetting styles. -
Flex Gap Property: The
gapproperty is not supported in the testing environment. Please replace it withmarginfor spacing in the.menu ulselector. -
CSS Variable for Blue Color: The task requires using a CSS variable for the blue color. Currently, the color
#00acdcis used directly. Please define and use a CSS variable for this color. -
Header Height: The task specifies setting a specific height for the header, but it seems padding is used instead. Ensure the header height aligns with the task requirements.
-
Image Source Path: The logo image path should be
src/imagesas per the task requirements, but it's currently set to./images/logo.png. Please correct this path.
These changes are essential to meet the core requirements of the task. Once these are addressed, we can move forward with approval. 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.
src/index.html
Outdated
| <h1>Moyo header</h1> | ||
| <header class="header"> | ||
| <div class="logo"> | ||
| <a href="#"> |
There was a problem hiding this comment.
Consider adding a more descriptive href attribute for the logo link to improve accessibility and SEO. Currently, it is set to #, which doesn't provide meaningful navigation.
src/style.css
Outdated
| align-items: center; | ||
| } | ||
|
|
||
| .menu .blue { |
There was a problem hiding this comment.
The .blue class is defined here, which is used in the HTML file. Ensure that this color choice aligns with your overall design theme.
src/style.css
Outdated
| color: #00acdc; | ||
| } | ||
|
|
||
| .menu .is-active::after { |
There was a problem hiding this comment.
The .is-active class is styled with a pseudo-element for an underline effect. Verify that this effect is visually appealing and consistent with your design intentions.
|
що не так? |
src/style.css
Outdated
| align-items: center; | ||
| } | ||
|
|
||
| .menu ul { |
There was a problem hiding this comment.
same - add classes to ul, li, a and add styles to them directly, using class selector
There was a problem hiding this comment.
I don't think it's appropriate to add classes to li and a because there are a lot of them
src/style.css
Outdated
| margin: 0; | ||
| } | ||
|
|
||
| .menu li { |
There was a problem hiding this comment.
i have to add classes to each li element and to each a element?
src/style.css
Outdated
| position: absolute; | ||
| left: 50%; | ||
| bottom: -0.2px; | ||
| width: 35px; |
There was a problem hiding this comment.
| width: 35px; | |
| width: 100%; |
src/style.css
Outdated
| content: ''; | ||
| position: absolute; | ||
| left: 50%; | ||
| bottom: -0.2px; |
There was a problem hiding this comment.
| bottom: -0.2px; | |
| bottom: 0; |
src/style.css
Outdated
| .menu .is-active::after { | ||
| content: ''; | ||
| position: absolute; | ||
| left: 50%; |
There was a problem hiding this comment.
| left: 50%; | |
| left: 0; |
src/style.css
Outdated
| height: 4px; | ||
| border-radius: 8px; | ||
| background-color: #00acdc; | ||
| transform: translateX(-50%); |
There was a problem hiding this comment.
| transform: translateX(-50%); |
| </head> | ||
| <body> | ||
| <h1>Moyo header</h1> | ||
| <header class="header"> | ||
| <div class="logo"> |
There was a problem hiding this comment.
Redundant wrapper
| <div class="logo"> |
