Solution #5813
Solution #5813
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While you've done a great job with the structure and styling, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Alt Attribute: The
altattribute for the image is not descriptive enough. It's important for accessibility that this attribute accurately describes the image content. Please update it to provide a more meaningful description. -
CSS for Active Link: The
::afterpseudo-element should be used for the blue line below the active link, not just on hover. This is crucial for indicating the current active page to users. -
Link Functionality: The
hrefattributes for navigation links and the logo are currently set to#. If these are meant to navigate to specific sections or the homepage, they should be updated with the correct URLs to ensure proper navigation.
These issues are important for both functionality and accessibility, so addressing them will greatly enhance the quality of your project. Keep up the great work, and I look forward to seeing the improvements! 🚀
✨ 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.
| <div class="header-logo"> | ||
| <a href="#"> | ||
| <img | ||
| src="images/logo.png" |
There was a problem hiding this comment.
Consider providing a more descriptive alt attribute for the logo image. This will improve accessibility for users who rely on screen readers.
src/index.html
Outdated
| <h1>Moyo header</h1> | ||
| <header> | ||
| <div class="header-logo"> | ||
| <a href="#"> |
There was a problem hiding this comment.
The href attribute for the logo link is currently set to #. If this link is intended to navigate to the homepage, consider setting it to the actual homepage URL.
src/index.html
Outdated
| <a href="#">Apple</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="#">Samsung</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="#">Smartphones</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a | ||
| href="#" | ||
| data-qa="hover" | ||
| class="is-active" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="#">Gadgets</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="#">Tablets</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="#">Photo</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="#">Video</a> |
There was a problem hiding this comment.
The href attributes for the navigation links are all set to #. If these links are meant to navigate to specific sections or pages, consider updating them with the correct URLs.
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| padding: 0 40px; |
There was a problem hiding this comment.
Consider adding a background-color to the header to ensure it stands out against the rest of the page content.
|
|
||
| .link-item a { | ||
| text-decoration: none; | ||
| color: #000; |
There was a problem hiding this comment.
The color for the links is set to #000, which is black. Ensure that this provides enough contrast against the background for accessibility purposes.
| content: ''; | ||
| position: absolute; | ||
| bottom: 0; | ||
| border-bottom: 4px solid var(--main-blue-color); |
There was a problem hiding this comment.
The border-bottom property is used to create an underline effect on hover. Ensure that this effect is visually appealing and consistent with the overall design of the site.
There was a problem hiding this comment.
Great job on addressing the previous issues! 🎉 You've successfully updated the alt attribute to "Moyo logo," enhancing accessibility. The ::after pseudo-element is now correctly used for the active link, improving user navigation. While the href attributes are placeholders, ensure they point to the correct URLs for full functionality. Keep up the good work and continue refining your skills! Remember, addressing these critical issues will greatly enhance the quality of your projects.
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <a href="/"> | ||
| <img | ||
| src="images/logo.png" | ||
| alt="Moyo logo" |
There was a problem hiding this comment.
The alt attribute for the image is not descriptive enough. It's important for accessibility that this attribute accurately describes the image content. Please update it to provide a more meaningful description.
| <a href="/"> | ||
| <img | ||
| src="images/logo.png" | ||
| alt="Moyo logo" | ||
| /> | ||
| </a> | ||
| </div> | ||
| <nav class="navigation"> | ||
| <ul class="nav-link-list"> | ||
| <li class="link-item"> | ||
| <a href="/">Apple</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="/">Samsung</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="/">Smartphones</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a | ||
| href="/" | ||
| data-qa="hover" | ||
| class="is-active" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="/">Gadgets</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="/">Tablets</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="/">Photo</a> | ||
| </li> | ||
| <li class="link-item"> | ||
| <a href="/">Video</a> |
There was a problem hiding this comment.
The href attributes for navigation links and the logo are currently set to /. If these are meant to navigate to specific sections or the homepage, they should be updated with the correct URLs to ensure proper navigation.
| .link-item a:hover, | ||
| .link-item a.is-active, | ||
| .link-item a:active{ | ||
| color: var(--main-blue-color); | ||
| } | ||
|
|
||
| .link-item a:hover::after, | ||
| .link-item a:active::after { |
There was a problem hiding this comment.
The ::after pseudo-element should be used for the blue line below the active link, not just on hover. This is crucial for indicating the current active page to users. Please ensure that the active link styling includes the ::after pseudo-element.
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 ❗️