Develop #7279
Develop #7279
Conversation
Demtar23
commented
Nov 24, 2025
Demtar23
commented
- DEMO LINK
- TEST REPORT LINK
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good job setting up the basic layout with flexbox! However, several changes are needed to meet the project's requirements. Your HTML structure is currently invalid because the <header> element must be placed inside the <body>. Please also adjust the Google Fonts import to load only the medium (500) weight as specified. [CHECKLIST ITEM #8]
The styling for the active and hover states needs correction. The blue underline must be implemented using the ::after pseudo-element on the .is-active class, and the hover effect should change the link's text color. [CHECKLIST ITEM #6] Additionally, the data-qa attribute on the fourth link should have the value hover, not hover nav-item. Finally, please remember to avoid using the universal (*) and tag selectors in your CSS, as this is against the project's style guide.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100..900;1,100..900&display=swap" | ||
| rel="stylesheet" | ||
| /> |
There was a problem hiding this comment.
This violates checklist item #8: 'The Google Fonts Configuration follows requirements.' You have imported a wide range of font weights and styles, but the task requires only the roman style with medium (500) weight. Please adjust the font URL to load only the specific font variant needed.
| <header class="header"> | ||
| <a | ||
| href="index.html" | ||
| class="logo" | ||
| > | ||
| <img | ||
| src="images/logo.png" | ||
| alt="Moyo" | ||
| /> | ||
| </a> | ||
| <nav> | ||
| <ul class="nav-list"> | ||
| <li> | ||
| <a | ||
| href="#" | ||
| class="is-active nav-item" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li><a href="#">Samsung</a></li> | ||
| <li><a href="#">Smartphones</a></li> | ||
| <li> | ||
| <a | ||
| href="#" | ||
| data-qa="hover nav-item" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li><a href="#">Gadgets</a></li> | ||
| <li><a href="#">Tablets</a></li> | ||
| <li><a href="#">Photo</a></li> | ||
| <li><a href="#">Video</a></li> | ||
| </ul> | ||
|
|
||
| <div class="nav-line"></div> | ||
| </nav> | ||
| </header> |
There was a problem hiding this comment.
The <header> element should be placed inside the <body> element. Currently, it is a sibling to <body>, which is structurally incorrect for an HTML document.
| <li> | ||
| <a | ||
| href="#" | ||
| class="is-active nav-item" |
There was a problem hiding this comment.
The nav-item class is applied to this link, but it's missing from most other navigation links. This will lead to inconsistent styling. For consistent styling, a common class should be applied to all navigation links (or their parent <li> elements).
| <li> | ||
| <a | ||
| href="#" | ||
| data-qa="hover nav-item" |
There was a problem hiding this comment.
The value of the data-qa attribute should be just hover. The additional nav-item text here is incorrect and will likely cause automated tests to fail.
| <li><a href="#">Video</a></li> | ||
| </ul> | ||
|
|
||
| <div class="nav-line"></div> |
There was a problem hiding this comment.
This <div> element is unnecessary. The requirement specifies using a CSS pseudo-element to create the blue line under the active navigation link, so this extra HTML element should be removed.
| padding: 0 20px; | ||
| } | ||
|
|
||
| .logo img { |
There was a problem hiding this comment.
The code style rules forbid using tag names for styling (except for html and body). You should add a class to the img element and use that class to apply styles.
| margin-right: 20px; | ||
| } | ||
|
|
||
| .nav-item:first-child::before { |
There was a problem hiding this comment.
This violates checklist item #6: 'Pseudo-element is used for a blue line below the active link'.
The implementation for the active link's underline is incorrect. The requirements specify using the ::after pseudo-element and targeting the .is-active class. This rule incorrectly uses ::before and targets :first-child.
| border-radius: 8px; | ||
| } | ||
|
|
||
| .nav-list a { |
There was a problem hiding this comment.
Styling elements by tag name (a) is against the code style rules. All navigation links should have a common class for styling.
| margin-right: 0; | ||
| } | ||
|
|
||
| .nav-list a:hover::after { |
There was a problem hiding this comment.
The requirement is that the link color should change on hover. This rule only adds a blue line using ::after, but it doesn't change the text color of the link. You should add a color property to a .nav-item:hover rule.
| .nav-list li:first-child a, | ||
| .nav-list li:nth-child(4) a { | ||
| color: var(--blue) !important; |
There was a problem hiding this comment.
Styling elements based on their position with :first-child or :nth-child() is not the correct approach here. The active link's style should be applied using the .is-active class. The color for the fourth link seems to be for demonstrating a hover state, which should be handled by a :hover pseudo-class instead.
2 participants
