Develop #5844
Develop #5844
Conversation
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 Your code is well-structured and meets most of the core requirements. While there are a few areas for improvement, such as using a more descriptive alt attribute for the logo image and considering semantic HTML5 elements for better accessibility, these are minor and do not impact the overall functionality. Keep up the good work and consider these suggestions for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <a | ||
| href="#" | ||
| class="logo" | ||
| > | ||
| <img | ||
| src="images/logo.png" | ||
| alt="logo" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
Consider adding a more descriptive alt attribute for the logo image to improve accessibility. Instead of just 'logo', you could use something like 'Company Logo' or 'Moyo Logo'.
| <header> | ||
| <a | ||
| href="#" | ||
| class="logo" | ||
| > | ||
| <img | ||
| src="images/logo.png" | ||
| alt="logo" | ||
| /> | ||
| </a> | ||
| <nav> | ||
| <ul> | ||
| <li> | ||
| <a | ||
| href="/apple" | ||
| class="is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li><a href="/samsung">Samsung</a></li> | ||
| <li><a href="/smartphones">Smartphones</a></li> | ||
| <li> | ||
| <a | ||
| href="/laptops" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li><a href="/gadgets">Gadgets</a></li> | ||
| <li><a href="/tablets">Tablets</a></li> | ||
| <li><a href="/photo">Photo</a></li> | ||
| <li><a href="/video">Video</a></li> | ||
| </ul> | ||
| </nav> | ||
| </header> |
There was a problem hiding this comment.
For better accessibility, consider using semantic HTML5 elements such as <nav> for the navigation bar instead of a generic <div> or <header>. This will help screen readers and other assistive technologies understand the structure of the page better.
| width: 100%; | ||
| height: auto; |
There was a problem hiding this comment.
Setting the width of the logo image to 100% might cause it to stretch if the container size changes. Consider setting a max-width instead to maintain the aspect ratio.
|
|
||
| a { | ||
| text-transform: uppercase; | ||
| color: #000; |
There was a problem hiding this comment.
The default link color is set to black (#000). Ensure this provides enough contrast against the background for readability and accessibility. Consider using a color contrast checker to verify this.
|
|
||
| a:hover, | ||
| a.is-active { | ||
| color: var(--link-color); |
There was a problem hiding this comment.
Using CSS variables for colors is a good practice. Ensure that --link-color provides sufficient contrast against the background when used for active and hover states.
DEMO LINK
TEST REPORT LINK
[ x] Header height is set in 1 place (for the links)
[ x] Content is vertically centered (for any header height)
[x ] CSS is used to show all letters in Uppercase (don't type them in HTML)
[x ] Logo is an image wrapped with a link
[x ] CSS Variable is used for a blue color
[x ] Pseudo-element is used for a blue line below the active link
[x ] Code follows all the Code Style Rules ❗️