firstTry #5849
firstTry #5849
Conversation
WhiteP8wder8
commented
Dec 20, 2024
WhiteP8wder8
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 a great start, there are a few critical issues that need to be addressed before we can approve the pull request:
-
CSS Issues:
- The
::afterpseudo-element for the.is-activeclass is not implemented correctly. It should visually indicate the active link with a blue line, but this is not currently working as intended. Please ensure it is positioned correctly using properties likeposition,bottom, andleft. - The
.nav_linkiclass seems redundant and is not required by the task. Consider removing it to simplify your CSS.
- The
-
HTML Issues:
- The
hrefattributes for all navigation links are currently the same. If these links are meant to navigate to different sections or pages, please update thehrefattributes accordingly. - The
altattribute for the logo image should be more descriptive for accessibility purposes. Consider changing it to something like 'MOYO company logo'.
- The
-
General Requirements:
- The header height and vertical centering of content need adjustments to meet the task requirements.
- Ensure that CSS is used to display text in uppercase, rather than typing it in uppercase in the HTML.
- Review the use of semantic tags to ensure full adherence to best practices.
These changes are necessary to meet the core requirements of the task. Once you've made these adjustments, please resubmit your pull request. Keep up the great work, and don't hesitate to reach out if you have any questions! 🌟
✨ 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
| src="images/logo.png" | ||
| alt="MOYO" |
There was a problem hiding this comment.
The alt attribute for the logo image is set to 'MOYO'. While this is a good start, it's recommended to provide more descriptive text for accessibility purposes, such as 'MOYO company logo'.
src/index.html
Outdated
| href="index.html" | ||
| class="nav_link is-active" | ||
| > | ||
| APPLE | ||
| </a> | ||
| </li> | ||
| <li class="nav_item"> | ||
| <a | ||
| href="index.html" | ||
| class="nav_link" | ||
| > | ||
| SAMSUNG | ||
| </a> | ||
| </li> | ||
| <li class="nav_item"> | ||
| <a | ||
| href="index.html" | ||
| class="nav_link" | ||
| > | ||
| SMARTPHONES | ||
| </a> | ||
| </li> | ||
| <li class="nav_item"> | ||
| <a | ||
| href="index.html" | ||
| class="nav_link nav_linki" | ||
| data-qa="hover" | ||
| > | ||
| LAPTOPS & COMPUTERS | ||
| </a> | ||
| </li> | ||
| <li class="nav_item"> | ||
| <a | ||
| href="index.html" | ||
| class="nav_link" | ||
| > | ||
| GADGETS | ||
| </a> | ||
| </li> | ||
| <li class="nav_item"> | ||
| <a | ||
| href="index.html" | ||
| class="nav_link" | ||
| > | ||
| TABLETS | ||
| </a> | ||
| </li> | ||
| <li class="nav_item"> | ||
| <a | ||
| href="index.html" | ||
| class="nav_link" | ||
| > | ||
| PHOTO | ||
| </a> | ||
| </li> | ||
| <li class="nav_item"> | ||
| <a | ||
| href="index.html" |
There was a problem hiding this comment.
All navigation links have the same href attribute pointing to 'index.html'. This might be intentional for a single-page application, but if these are meant to navigate to different sections or pages, the href attributes should be updated accordingly.
src/style.css
Outdated
| .is-active::after { | ||
| /* I don't now how I can do this */ | ||
| content: ''; | ||
| width: 100%; | ||
| height: 10px; | ||
| background-color: #00acdc; | ||
| border-radius: 30px; | ||
| text-decoration: underline; |
There was a problem hiding this comment.
The .is-active::after pseudo-element is intended to add a visual indicator for active links, but the implementation is incomplete. Consider positioning the pseudo-element correctly using properties like position, bottom, and left to ensure it appears as intended.
src/style.css
Outdated
| .nav_linki { | ||
| color: #00acdc; | ||
| } |
There was a problem hiding this comment.
The .nav_linki class appears to be redundant since it only changes the color to #00acdc, which is already handled by the .nav_link:hover and .is-active classes. Consider removing this class to simplify the CSS.
