Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this can be considered complete, mainly because several explicit checklist rules are still violated and the linter is failing.
Critical issues to fix (blockers):
- In
src/style.cssyou use the universal selector reset* { margin: 0; padding: 0; box-sizing: border-box; }, which is disallowed; replace this with targeted resets (e.g.html, body { margin: 0; }, specific elements likeul,h1,p, andhtml { box-sizing: border-box; }) so you comply with [CHECKLIST ITEM #16]. - The selector
.logo imginsrc/style.cssand the corresponding markup insrc/index.htmlviolate [CHECKLIST ITEM #17]; add a class to the image (e.g.class="logo__img") and style.logo__imginstead of using theimgtag name in the selector. - Header height rules are broken:
.headerhasheight: 60pxand the nav links also have a height, so height is defined twice and on the wrong element; remove the height from.headerand keep a singleheightdeclaration only on the nav links to satisfy [CHECKLIST ITEM #4] and [CHECKLIST ITEM #30]. - All
.nav__linkelements currently havemargin-left: 20px;, which creates unwanted space before the first item; instead, apply the margin only between items (e.g..nav__list > li + li { margin-left: 20px; }or a:not(:first-child)rule) to meet [CHECKLIST ITEM #28]. - The linter error in
src/style.cssaround line 15 (font-family: "Roboto", sans-serif;) must be fixed by removing the unnecessary quotes (font-family: Roboto, sans-serif;), and the required TEST REPORT LINK (https://<your_account>.github.io/layout_moyo-header/report/html_report/) needs to be added to the PR description as per [CHECKLIST ITEM #33].
Minor improvements (non-blockers, but recommended):
- Explicitly center the nav link text horizontally (for example using
display: flex; align-items: center; justify-content: center;on the links) and adjust any over‑80‑character HTML lines (like the Google Fonts link) to respect the line‑length style rule.
You’ve done a solid job on the layout, flexbox usage, Roboto configuration, active-link styling, and semantic structure; once you address the above points, your solution should fully meet the task and pass all automated checks.
✨ 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
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
This violates checklist item #16: "Do not use the universal * selector in CSS." You currently use * { margin: 0; padding: 0; box-sizing: border-box; } (reset) which is disallowed. Reset browser defaults without * (for example, set margin:0 on html, body and target specific elements like ul, h1, p that need reset, or add a dedicated .reset class).
| .logo img { | ||
| height: 40px; | ||
| display: block; |
There was a problem hiding this comment.
This violates checklist item #17: "Do not use tag names for styling in CSS (except html and body); style elements using classes and, if needed, pseudo-classes, pseudo-elements, or attribute selectors." The selector .logo img uses a tag name. Add a class to the image (for example .logo__img) and style that class instead of using the img tag in the selector.
| .header { | ||
| height: 60px; /* важно — в одном месте */ | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| padding: 0 50px; |
There was a problem hiding this comment.
This violates checklist item #4: "The height must be set for the nav links (not for the header), and this height must be taken from the design." You set height: 60px; on .header. Move the height declaration to the nav links (only) and remove it from the header so the header height is determined by its content.
| height: 60px; /* важно — в одном месте */ | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| padding: 0 50px; | ||
| } | ||
|
|
||
| /* ЛОГО */ | ||
| .logo img { | ||
| height: 40px; | ||
| display: block; | ||
| } | ||
|
|
||
| /* СПИСОК */ | ||
| .nav__list { | ||
| display: flex; | ||
| list-style: none; | ||
| } | ||
|
|
||
| /* ССЫЛКИ */ | ||
| .nav__link { | ||
| display: flex; | ||
| align-items: center; | ||
| height: 60px; |
There was a problem hiding this comment.
This violates checklist item #30: "Header height must be set in exactly one place (for the links)." Height is currently defined on both .header and .nav__link (height: 60px; at the header and again on the links). Keep the height in exactly one place (the nav links) and remove the duplicate from the header.
| display: flex; | ||
| align-items: center; | ||
| height: 60px; | ||
| margin-left: 20px; |
There was a problem hiding this comment.
This violates checklist item #28: "There must not be margins before the first and after the last list items in the navigation list." You apply margin-left: 20px; to every .nav__link, which adds spacing before the first item. Instead, apply spacing between items only (for example, use .nav__list > li + li { margin-left: 20px; }), or use :not(:first-child) so the first item has no left margin.
| <img src="./images/logo.png" alt="Moyo logo"> | ||
| </a> | ||
|
|
||
| <nav class="nav"> |
There was a problem hiding this comment.
You set a height on .header here (height: 60px). The checklist requires: "Header height is set in 1 place (for the links)" and the description says the height should be set for nav links (not for the header). Remove the height from .header and keep the fixed height only on the navigation links so the header height is defined in exactly one place.
| </a> | ||
| </li> | ||
| <li><a href="#" class="nav__link">Gadgets</a></li> |
There was a problem hiding this comment.
Selector .logo img uses a tag name in CSS which violates: "Do not use tag names for styling (except html and body)". Add a class to the <img> (for example logo__image) and style that class (e.g. .logo__image { height: 40px; display:block; }).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed because there are still checklist violations and test failures that block this task from being fully complete.
The main blocker is the use of the ul tag selector for styling in both src/style.css and the CSS inside src/index.html (the ul { margin: 0; padding: 0; } rule), which violates the rule "Don't use tag names for styling (except html and body)" [CHECKLIST ITEM #17]; please remove that rule and move the reset styles to the nav list class instead, e.g. .nav__list { margin: 0; padding: 0; }. Another checklist issue is the overly long Google Fonts <link> line in src/index.html (the href attribute makes the line exceed 80 characters); reformat that tag so each attribute is on its own line and no line goes over 80 characters while keeping the same font configuration. In addition, BackstopJS visual regression tests for the header and nav (Header tag, Nav tag, Link with class_is-active, Link with data-qa_hover) are failing with significant mismatches, which means the layout, spacing, and/or typography of your header and navigation still differ from the reference; you should run npm test, open the Backstop HTML report, and adjust header/nav sizes, spacing between links, active/hover underline, and alignment until all scenarios pass.
Minor, non-blocking improvements you can make while revisiting the code are: adding blank lines between multi-line sibling elements in the HTML for readability, and optionally refining the logo alt text to something slightly more descriptive. Overall, your structure, semantic HTML, and use of flexbox and CSS variables are solid; once you fix the selector misuse, the long line, and bring the visuals in line with the Backstop reference, this implementation will be in great 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
| ul { | ||
| margin: 0; | ||
| padding: 0; |
There was a problem hiding this comment.
This CSS block uses the ul tag selector which violates the checklist rule: "Don't use tag names for styling (except html and body)". Target the nav list with its class (for example .nav__list { margin: 0; padding: 0; }) instead of styling the ul tag directly so you comply with [CHECKLIST ITEM #17].
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.gstatic.com" |
There was a problem hiding this comment.
This CSS rule uses a tag selector for ul. The checklist requires styling by classes (only html and body may use tag selectors). Quote from the checklist: "Don't use tag names for styling (except html and body)". Move these resets to the .nav__list class (for example, .nav__list { margin: 0; padding: 0; }) and remove the ul { ... } rule.
| crossorigin="anonymous" | ||
| > | ||
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap" |
There was a problem hiding this comment.
This HTML line is longer than 80 characters. The checklist rule states: "Lines of code have 80 chars max". The Google Fonts <link> href value is very long (line 16). Please reformat so no line exceeds 80 characters (for example, ensure each attribute is on its own line and adjust the wrapping to keep lines <= 80 chars) while keeping the correct Google Fonts configuration.
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 keep progressing while you fix a few remaining checklist issues. You’ve done a solid job on the header structure and behavior (flexbox layout, centered nav links, active/hover states, and logo setup), which shows good understanding of the core requirements. The most important fixes are in src/index.html: at line 1 make sure the doctype is exactly <!DOCTYPE html> with nothing (not even a stray href line) before it, and move the Google Fonts loading into a <link> inside <head> with each attribute on its own line and no line over 80 characters; then remove the @import Google Fonts line from src/style.css line 1. In src/style.css, the selector .nav__list > li + li and the matching <li> elements in src/index.html violate the “no tag-name styling” rule [CHECKLIST ITEM #17], so add a class like class="nav__item" to each <li> and change the selector to something like .nav__list > .nav__item + .nav__item. As a smaller improvement, scan both files for any lines longer than 80 characters (long HTML attributes or CSS rules) and wrap them onto multiple lines for readability and to satisfy the line-length rule. Overall, you’re very close—these are mostly formatting and style refinements, and addressing them will strengthen both your code quality and your toolkit for future projects.
✨ 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
| @@ -1,22 +1,104 @@ | |||
| href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap" | |||
There was a problem hiding this comment.
This line is invalid HTML: a bare href before the doctype must be removed. Instead add a properly formatted Google Fonts inside , with each attribute on its own line and no line longer than 80 characters. This violates the checklist item: "The Google Fonts Configuration follows requirements."
| @@ -1,22 +1,104 @@ | |||
| href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap" | |||
There was a problem hiding this comment.
Loading the font via @import here both causes a very long line and duplicates font loading. Move the font inclusion into a properly formatted tag in src/index.html (and remove this @import). Also ensure no line exceeds 80 characters: this line currently violates "Lines of code have 80 chars max" and the Google Fonts requirement.
|
|
||
| <li> | ||
| <a | ||
| href="#" |
There was a problem hiding this comment.
The selector .nav__list > li + li uses the li tag name in CSS. The project rules forbid tag-name styling: "Don't use tag names for styling (except html and body)". Add a semantic class to each
nav__item) and change the selector to use that class (e.g. .nav__list > .nav__item + .nav__item).|
|
||
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li> |
There was a problem hiding this comment.
Your
nav__item). Because of the rule "Don't use tag names for styling (except html and body)", add a semantic class to every li tag.| @@ -1,3 +1,74 @@ | |||
| @import url(https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap); | |||
There was a problem hiding this comment.
Line 1 is an invalid standalone href line before the DOCTYPE. This violates the checklist item: "The Google Fonts Configuration follows the specified requirements." Also it breaks the rule from the formatting checklist: "Keep your attributes correctly formatted" and causes a line that likely exceeds the "Lines of code have 80 chars max" rule. Remove this stray line and add the Google Fonts <link> inside the <head> instead. Format the <link> so each attribute is on its own line (keep the same font query: Roboto wght@500) and make sure no line exceeds 80 characters.
| @@ -1,3 +1,74 @@ | |||
| @import url(https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap); | |||
There was a problem hiding this comment.
You're importing Google Fonts in CSS via @import (style.css line 1). The task and previous review expect the font to be included via a properly formatted <link> in src/index.html (see: "The Google Fonts Configuration follows the specified requirements"). Remove the @import from style.css, and add the <link> in the HTML <head> with each attribute on its own line so it follows the attribute-formatting and 80-char rules.
brespect
left a comment
There was a problem hiding this comment.
Almost done, but your DEMO LINK & TEST REPORT LINK are not available
brespect
left a comment
There was a problem hiding this comment.
Good progress, but your DEMO LINK and TEST REPORT LINK seem to be broken
DEMO LINK: https://lilbodyn.github.io/layout_moyo-header/
TEST REPORT LINK: https://lilbodyn.github.io/layout_moyo-header/report/html_report/
[x] Header height is set in 1 place
[x] Content is vertically centered
[x] CSS uppercase used
[x] Logo is a link
[x] CSS variable used
[x] ::after used
[x] Code style ok
[x] Google font used