Conversation
ArtemKalchenko1
commented
Apr 11, 2026
ArtemKalchenko1
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
I am requesting changes because several required checklist items are missing or implemented incorrectly, and these are currently blocking the automated tests from passing. The main issues are: there is no <nav> element wrapping your <ul class="nav-menu"> in src/index.html (required semantic structure, [CHECKLIST ITEM #3]), the active link uses class="isActive" instead of the exact is-active name, and the 4th link (Laptops & Computers) is missing the data-qa="hover" attribute (both required for tests, [CHECKLIST ITEMS #9 and #10]). In src/style.css, you are using gap on the flex container for spacing (.nav-menu { gap: ... }), which must be replaced with margins on the list items ([CHECKLIST ITEM #13]), and the blue underline for the active link is done with border-bottom and padding instead of a ::after pseudo-element on the .is-active link with no padding on links ([CHECKLIST ITEMS #12, #16, #26]). The links also need a consistent height set in one place (on the link, not the header) and vertical centering adjusted accordingly ([CHECKLIST ITEMS #6 and #21]), and font-weight: 500px; must be corrected to font-weight: 500; so the Roboto embedding is valid ([CHECKLIST ITEMS #2 and #28]). Additionally, your CSS currently styles anchor tags directly (e.g. .nav-menu a, a.isActive) and does not use text-transform: uppercase; for the nav text, so move these styles to a link class like .nav-menu__link and apply uppercase there, then target .nav-menu__link.is-active for the active state ([CHECKLIST ITEMS #23 and #40]). As minor improvements, you can tidy HTML formatting by adding blank lines between multi-line <li> blocks and wrapping very long lines to stay under 80 characters, but these are not test blockers. Once you address the semantic <nav> element, class/attribute names, spacing without gap, correct pseudo-element underline, link height/centering, and font-weight, your solution should be in a good position to pass the tests and match the design much more closely.
✨ 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
| padding: 10px 50px; | ||
| } | ||
|
|
||
| .nav-menu { |
There was a problem hiding this comment.
The markup uses a plain
- for navigation but the checklist requires the semantic nav element. Quote: "use semantic tags:
<header>, <img>, <nav>, <ul>, <li> and <a>" [CHECKLIST ITEM #3]. Wrap the list inside a (keep the logo outside the nav).| display: flex; | ||
| list-style: none; | ||
| gap: 20px; | ||
| margin: 0; |
There was a problem hiding this comment.
The active link class is incorrect. Quote: "❗️ the blue link with a line below should have a class is-active in addition to any other classes you add" [CHECKLIST ITEM #9]. Replace isActive with the exact is-active class name.
|
|
||
| .nav-menu a:hover { | ||
| color: var(--blue); | ||
| } |
There was a problem hiding this comment.
The 4th link is missing the required test attribute. Quote: "❗️ add data-qa="hover" attribute to the 4th link for testing (Laptops & computers)" [CHECKLIST ITEM #10]. Add data-qa="hover" to the 4th anchor element.
| .nav-menu { | ||
| display: flex; | ||
| list-style: none; | ||
| gap: 20px; |
There was a problem hiding this comment.
The CSS uses the flex gap property, but tests forbid it. Quote: "don't use the flex gap property for indents. It's not supported in tests, use margin instead" [CHECKLIST ITEM #13]. Replace gap with spacing via margins on the list items.
| padding: 0; | ||
| } | ||
|
|
||
| .nav-menu a { |
There was a problem hiding this comment.
Link styling should make text uppercase via CSS and you should avoid styling by tag selectors. Quote: "CSS is used to show all letters in Uppercase (don't type them in HTML)" [CHECKLIST ITEM #23] and "Don't use tag names for styling (except html and body)" [CHECKLIST ITEM #40]. Move a styling to a class-based selector (e.g. .nav-menu__link) and add text-transform: uppercase; there.
| <ul class="nav-menu"> | ||
| <li> | ||
| <a | ||
| href="#" |
There was a problem hiding this comment.
This violates checklist item #13: "don't use the flex gap property for indents. It's not supported in tests, use margin instead" — you used gap: 20px on .nav-menu. Remove gap and add horizontal margins to list items (taking care to avoid margins before the first and after the last items).
| > | ||
| Apple | ||
| </a> | ||
| </li> |
There was a problem hiding this comment.
This violates multiple checklist items: #40 "Don't use tag names for styling (except html and body)", #23 "CSS is used to show all letters in Uppercase (don't type them in HTML)", and #6 "the height should be set for nav links (not the header), take it from the design" — you target links via .nav-menu a (tag used) and there is no text-transform: uppercase nor a height set on links. Use a class for links (e.g. .nav__link), add text-transform: uppercase, and set the link height in one place (not the header).
| <li> | ||
| <a | ||
| href="#" | ||
| > |
There was a problem hiding this comment.
This violates checklist item #40: "Don't use tag names for styling (except html and body)" — the selector a.isActive uses a tag name and also uses the wrong class name. Use a class-based selector like .nav__link.is-active instead of a.isActive.
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> |
There was a problem hiding this comment.
This violates checklist item #12: "use the ::after and position it relative to a link with is-active class" (and #26: pseudo-element for the line) — currently you use border-bottomon the active link. Replace that with a positioned::afterpseudo-element on the.is-active` link to draw the blue line (so tests that look for a pseudo-element pass).
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li> |
There was a problem hiding this comment.
This violates checklist item #16: "Nav Links should not have any padding but have the text centered" — you added padding-bottom: 18.5px to the active link. Remove padding from links and align text by setting link height and centering (line-height or flex) so spacing is consistent and height is defined in one place.
3 participants
