Conversation
Skay9n
commented
Jan 31, 2026
Skay9n
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
Great job on the overall HTML structure and the use of flexbox for the layout! The semantic tags are used correctly.
However, a few changes are needed to meet all the project's requirements. Here are the key issues to address:
- The header is not full-width because of the horizontal
marginapplied to it. The requirement is for the header element itself to span the entire page width, with spacing applied to the content inside it. - Several styles are applied using tag selectors (
header,img,li), which is against the project's coding standards. Please add classes to these elements and use class selectors for styling instead. - The blue color (
#00acdc) is hardcoded. This color should be defined as a CSS variable in the:rootand used throughout the stylesheet. [CHECKLIST ITEM #5]
Once these points are addressed, your solution will be ready for approval. Keep up the great work!
✨ 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
src/index.html
Outdated
| <li> | ||
| <a | ||
| href="/" | ||
| class="nav__link is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="/" | ||
| class="nav__link" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="/" | ||
| class="nav__link" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="/" | ||
| class="nav__link" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="/" | ||
| class="nav__link" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="/" | ||
| class="nav__link" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="/" | ||
| class="nav__link" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="/" | ||
| class="nav__link" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> |
There was a problem hiding this comment.
To fully adhere to the project's CSS code style guidelines, which prohibit styling by tag name, consider adding a class to each <li> element (e.g., class="nav__item"). This will enable you to style the list items in your CSS file without using the li tag selector.
src/style.css
Outdated
| margin: 0; | ||
| } | ||
|
|
||
| header { |
There was a problem hiding this comment.
According to the task requirements, you should not use tag names for styling (except for html and body). Please add a class to the <header> element in your HTML and use that class for styling.
src/style.css
Outdated
| font-weight: 500; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| margin: 0 50px; |
There was a problem hiding this comment.
The requirement states, 'the header should stretch the full page width (don't use a horizontal margin)'. Applying a horizontal margin here violates this. This rule, combined with width: 100%, also causes a horizontal scrollbar. The content inside the header should be spaced from the edges, not the header element itself.
src/style.css
Outdated
| width: 100%; | ||
| } | ||
|
|
||
| img { |
There was a problem hiding this comment.
Styling elements with tag selectors is not allowed as per the project guidelines. This img selector is too generic and will affect all images on the page. Please add a specific class to the logo image for styling.
src/style.css
Outdated
| color: black; | ||
| } | ||
|
|
||
| .nav__list li:not(:last-child) { |
There was a problem hiding this comment.
Using the li tag selector is against the project's coding standards. You should add a class to the list items (e.g., .nav__item) in your HTML and use that class to apply styles for the margin.
src/style.css
Outdated
| } | ||
|
|
||
| .nav__link:hover { | ||
| color: #00acdc; |
There was a problem hiding this comment.
This violates checklist item #5: 'CSS Variable is used for a blue color'. The color #00acdc is hardcoded. Please define a CSS variable for this color (e.g., in a :root selector) and use it here and in other rules where this color is needed.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this update! You've successfully addressed all the feedback from the previous review, and the solution is now in great shape. Your code is clean, well-structured, and meets all project requirements, from the full-width flexbox header to the correct use of CSS variables. I am approving your submission. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
