Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several explicit checklist requirements are not yet met, so the task is currently incomplete.
Critical issues that must be fixed:
- The logo image is wrapped in an
<a>without anhrefinsrc/index.html, so it is not a functional link as required by [CHECKLIST ITEM #14]; add an appropriatehref(for example/or#) to make it a real navigation link. - The blue color
#00acdcis hard-coded in multiple places in bothsrc/index.htmlandsrc/style.css, and there is no CSS variable like:root { --blue: #00acdc; }, which violates [CHECKLIST ITEM #26]; define the variable once and replace all direct uses of that hex withvar(--blue). - Navigation link hover behavior is only applied to the element with
data-qa="hover", so most links do not change color on:hover, breaking [CHECKLIST ITEM #9]; add a general hover rule (for example targeting.nav__link:hover) that uses the blue variable so every nav link visually responds on hover. - The CSS uses tag-based selectors such as
a.is-activeanda.is-active::afterin both HTML-embedded and external styles, which conflicts with [CHECKLIST ITEM #38]; change these to class-only selectors like.nav__link.is-activeand.nav__link.is-active::after, and ensure the active state still positions the pseudo-element correctly.
Minor but important improvements: some lines (for example the meta viewport tag and long <link> tags) appear to exceed 80 characters, which goes against [CHECKLIST ITEM #32], and your PR description is missing the copied checklist with - [x] marks as required by [CHECKLIST ITEM #25]; please wrap long attributes across lines and add the checklist block to your PR body.
Overall, the structure, flexbox layout, font setup, and most semantics look good, and once you address these specific checklist violations your solution should be ready to pass.
✨ 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
|
|
||
| .nav { | ||
| margin-right: 50px; | ||
| } |
There was a problem hiding this comment.
This violates checklist item #14: "Logo is an image wrapped with a link" — the anchor wrapping the logo has no href attribute. Make the logo anchor a real link (for example: href="/" or href="#") so it's a proper link element.
|
|
||
| a.is-active { | ||
| display: flex; | ||
| color: #00acdc; |
There was a problem hiding this comment.
This violates checklist item #26: "A CSS variable is used for the blue color." The blue color #00acdc is hard-coded in multiple places (e.g. here). Define a CSS variable (for example :root { --blue: #00acdc; }) and use var(--blue) instead of repeating the literal color.
| [data-qa='hover']:hover { | ||
| color: #00acdc; | ||
| } |
There was a problem hiding this comment.
This violates checklist item #9: "Link color should be changed on :hover." Currently only the 4th link (via [data-qa='hover']) changes color on hover. Add a general hover rule for navigation links (for example .nav__link:hover { color: var(--blue); }) so links change color on :hover while keeping the data-qa attribute for tests.
| a.is-active { | ||
| display: flex; | ||
| color: #00acdc; | ||
| position: relative; | ||
| } | ||
|
|
||
| [data-qa='hover']:hover { | ||
| color: #00acdc; | ||
| } | ||
|
|
||
| a.is-active::after { | ||
| display: flex; | ||
| content: ''; | ||
| position: absolute; | ||
| width: 100%; | ||
| height: 4px; | ||
| background-color: #00acdc; | ||
| bottom: 0; | ||
| border-radius: 8px; |
There was a problem hiding this comment.
This violates checklist item #38: "Don't use tag names for styling (except html and body); style all elements using .class ..." You use a.is-active and a.is-active::after in CSS. Replace these with class-only selectors like .is-active and .is-active::after (and ensure .is-active has position: relative) to comply with the rule.
| class="nav__link" | ||
| href="#smartphones" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
|
|
||
| <li class="list_item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#laptops&computers" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> |
There was a problem hiding this comment.
Checklist item #26 requires a CSS variable for the blue color: "A CSS variable is used for the blue color". Right now the blue hex #00acdc is hard-coded in multiple places (color and background-color). Define a variable (for example :root { --blue: #00acdc; }) and replace direct hex values with var(--blue).
| <li class="list_item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#smartphones" |
There was a problem hiding this comment.
This rule uses a tag selector: a.is-active. Checklist item #38 states: "Don't use tag names for styling (except html and body); style all elements using .class.... Change this to a class-only selector such as .nav__link.is-activeor.is-active`.
| <a | ||
| class="nav__link" | ||
| href="#laptops&computers" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
|
|
| </a> | ||
| </li> |
There was a problem hiding this comment.
Checklist item #9 requires: "Link color should be changed on :hover." Currently only the element with data-qa='hover' has a hover color. Add a hover rule for all nav links (for example .nav__link:hover { color: var(--blue); }) so every link changes color on hover.
| <body> | ||
| <h1>Moyo header</h1> | ||
| <header class="header"> | ||
| <a class="image"> |
There was a problem hiding this comment.
Checklist item #14: "Logo is an image wrapped with a link." The <a> wrapping the logo has no href attribute — add href (for example href="/" or href="#") so the element is a functional link.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because some explicit checklist requirements are still not met, even though the header functionality and layout are otherwise correct.
Critical issues to fix:
- In
src/index.html, the<meta name="viewport" ...>attributes and the Google Fonts<link>tag currently create lines that exceed the required 80-character limit; you should break these so that each attribute starts on its own line and each resulting line is under 80 characters (e.g. placehref,rel, andname/contenton separate wrapped lines) [CHECKLIST ITEM]. - Similarly, wherever you include these tags in other files (e.g. any header snippet in
src/style.cssor elsewhere), make sure long attribute values are wrapped so that no single line goes over 80 characters and the attributes follow the “one attribute per line” style from the checklist’s good example. - The task also requires you to copy the provided Checklist into your Pull Request description and mark each item with
- [x]after verifying it; please update the PR body to include this block so that the review process and automated checks can rely on it [CHECKLIST ITEM].
These are formatting/compliance issues rather than functional bugs, but they are explicitly called out by the task, so they need to be fixed before the solution can be accepted. Once you’ve wrapped the long attributes correctly and added the checklist to the PR description, your implementation should fully satisfy the requirements, as the structural and visual behavior of the header is already in good shape.
✨ 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
| font-family: Roboto, sans-serif; | ||
| font-size: 12px; | ||
| text-transform: uppercase; | ||
| font-weight: 500; |
There was a problem hiding this comment.
This violates checklist item "Lines of code have 80 chars max" and the checklist section "Keep your attributes correctly formatted" — the meta viewport tag value is split across lines and creates long lines. Wrap long attribute values and keep each attribute on its own line (see the Good Example in the checklist).
| justify-content: center; | ||
| align-items: center; | ||
| padding-left: 50px; | ||
| } |
There was a problem hiding this comment.
This violates checklist item "Lines of code have 80 chars max" and the checklist section "Keep your attributes correctly formatted" — the long Google Fonts href should be wrapped across lines (start each attribute on a new line as in the Good Example). Break the href/rel attributes so no line exceeds 80 characters.
| content="width=device-width, user-scalable=no, initial-scale=1.0, | ||
| maximum-scale=1.0, minimum-scale=1.0" |
There was a problem hiding this comment.
This meta viewport attribute block is split across lines but individual lines appear to be longer than allowed by the checklist. Please follow the checklist item: "Lines of code have 80 chars max" — wrap long attribute values / break attributes across lines so each line stays within 80 characters.
| crossorigin="anonymous" | ||
| /> | ||
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap" |
There was a problem hiding this comment.
The Google Fonts href line is long and may exceed 80 characters. Please follow the checklist item: "Lines of code have 80 chars max" — wrap long attributes across lines (start each attribute on a new line when needed) so no line exceeds 80 chars.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can continue progressing while you address a few remaining formatting and PR-setup issues. You did a strong job on the header itself: the semantic structure, flexbox layout, active link underline, hover state, CSS variable for the blue color, and uppercase styling all meet the functional requirements. The main issues to fix are in src/index.html, where the <meta name="viewport"> tag and the Google Fonts <link> tag in the <head> need to have each attribute on its own line and be wrapped so no individual line exceeds 80 characters, including splitting the long href value as needed [CHECKLIST ITEM #44, #45]. Apply the same one-attribute-per-line and 80-character wrapping rule anywhere else those tags appear (for example, any snippets or duplicate HTML files) so the style checks pass consistently. You also need to update the Pull Request description to add the DEMO and TEST REPORT links and paste in the full checklist block, marking each verified item with - [x] so reviewers and automated tools can rely on it [CHECKLIST ITEM #46]. Once you clean up those formatting and PR-description details, your implementation will fully align with the project expectations, and you should feel confident about the quality of your header work so far.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| content="width=device-width, user-scalable=no, | ||
| initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0" |
There was a problem hiding this comment.
This violates CHECKLIST ITEM #44: "In src/index.html, break the <meta name="viewport" ...> tag and the Google Fonts <link> tag so that: each attribute starts on its own line, and each resulting line is under 80 characters (e.g., place href, rel, and name / content on separate wrapped lines)."
The viewport meta's attributes/content currently produce a line that still exceeds the required 80-character limit / are not wrapped as requested. Please ensure each attribute starts on its own line and wrap long attribute values so every source line is <= 80 chars.
| /> | ||
| <link | ||
| rel="stylesheet" | ||
| href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap" |
There was a problem hiding this comment.
This violates CHECKLIST ITEM #44: "In src/index.html, break the <meta name="viewport" ...> tag and the Google Fonts <link> tag so that: each attribute starts on its own line, and each resulting line is under 80 characters (e.g., place href, rel, and name / content on separate wrapped lines)."
The Google Fonts <link> href is on a single long line (this line) and likely exceeds 80 characters. Wrap the href (or break it across lines) and place each attribute (e.g., rel, href, crossorigin) on its own line so no line exceeds 80 characters.
| </head> | ||
| <body> | ||
| <h1>Moyo header</h1> | ||
| <header class="header"> |
There was a problem hiding this comment.
This violates CHECKLIST ITEM #46: "Update the Pull Request description to include the provided Checklist block, with each item marked - [x] after verification, so the review process and automated checks can rely on it."
Please update the PR description to include the DEMO and TEST REPORT links and copy the full Checklist block into the PR body, marking completed items with - [x].
| font-size: 12px; | ||
| text-transform: uppercase; | ||
| font-weight: 500; | ||
| } | ||
|
|
There was a problem hiding this comment.
The tag spans multiple lines but some resulting lines are still too long. Per checklist item #44 break this tag so each attribute starts on its own line and ensure no line exceeds 80 characters (wrap long attribute values accordingly).
| padding-left: 50px; | ||
| } | ||
|
|
||
| .logo { |
DEMO LINK https://punana990.github.io/MOYO-Header/
TEST REPORT LINK https://punana990.github.io/MOYO-Header/report/html_report/