Skip to content

Develop#7737

Open
levchenkoeva wants to merge 2 commits intomate-academy:masterfrom
levchenkoeva:develop
Open

Develop#7737
levchenkoeva wants to merge 2 commits intomate-academy:masterfrom
levchenkoeva:develop

Conversation

@levchenkoeva
Copy link
Copy Markdown

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am asking for changes because there are a few checklist violations and visual differences that are considered blocking for this task, even though most of the requirements are implemented correctly.

The critical issues are:

  • In src/style.css near the top, you use the universal selector * { ... } to reset margins/padding/box-sizing, which is explicitly disallowed by the Code Style Rules [CHECKLIST ITEM #7]; instead, reset only specific elements like html, body (and maybe img) with margin: 0; padding: 0; box-sizing: border-box;.
  • In src/style.css, the rule .nav_item:not(:first-child) currently includes margin-bottom: 16px, which creates an unwanted margin after the last <li> and violates the requirement that there must be no margins before the first and after the last list items; remove this vertical margin and keep only horizontal spacing (e.g. margin-left or .nav_item + .nav_item { margin-left: 30px; }).
  • You define .logo { height: 40px; } in src/style.css and also set height: 15px; on .nav_link; the header height must be controlled in one place via the nav links only [CHECKLIST ITEM #1], so remove the explicit height from .logo (or otherwise ensure only the link height determines the header height).
  • The .logo class is defined in src/style.css but not applied to the <img> in src/index.html (the image lacks class="logo"), so your intended logo sizing isn’t used and this contributes to the Backstop visual diffs; either add class="logo" to the <img> or adjust the selector so the image is correctly styled.

As a minor clean-up, you can remove the redundant position: relative from .is-active since .nav_link already has it, which keeps the CSS clearer. Once you address these points, your solution should both satisfy the checklist and pass the visual tests, and the rest of your implementation (semantic structure, flex layout, Roboto usage, active/hover states) is on the right track—good work so far.


✨ 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. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.html
Comment on lines +35 to +36
<img
src="./images/logo.png"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You defined .logo in CSS but the image in HTML has no class. The logo styles (height, margin) won't apply. Add the class to the image or the wrapping link (for example <img class="logo" ...>), or update the CSS to target the element actually used.

Comment thread src/index.html
Comment on lines +35 to +37
<img
src="./images/logo.png"
alt="Moyo logo"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule adds a vertical margin to many list items (margin-bottom: 16px) which creates a margin after the last item. The requirements state: "There should not be margins before the first and after the last list items." Remove the margin-bottom and keep only the horizontal spacing (use margin-left on not-first items or the sibling selector).

Comment thread src/index.html
Comment on lines +24 to +46
href="https://fonts.gstatic.com"
crossorigin="anonymous"
/>
<link
href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap"
rel="stylesheet"
/>
</head>
<body>
<h1>Moyo header</h1>
<header class="header">
<a href="#">
<img
src="./images/logo.png"
alt="Moyo logo"
/>
</a>
<nav class="nav">
<ul class="nav_list">
<li class="nav_item">
<a
class="nav_link is-active"
href="#"
>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #1: "Header height is set in 1 place (for the links)" — you set height: 15px on .nav_link but also set height: 40px on .logo. The task requires the header height to be driven by the nav links only (set in a single place). Remove the extra height declaration (for example remove .logo height) or adjust so only the nav links determine header height.

Comment thread src/index.html
Comment on lines +50 to +55
<li class="nav_item">
<a
class="nav_link"
href="#"
>
Samsung
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor redundancy: .nav_link already has position: relative so adding position: relative again on .is-active is unnecessary. You can remove the duplicate position: relative from .is-active to keep CSS cleaner.

Comment thread src/style.css
Comment on lines +1 to +5
* {
margin: 0;
padding: 0;
box-sizing: border-box;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #7: "Code follows all the Code Style Rules ❗️". The stylesheet uses the universal selector * to reset margin/padding at these lines, but the Code Style Rules explicitly disallow the * selector. Replace this reset with a permitted approach (for example html, body { margin: 0; padding: 0; }) or reset only the specific elements you need.

Comment thread src/style.css
Comment on lines +35 to +37
.nav_item:not(:first-child) {
margin-left: 30px;
margin-bottom: 16px;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should not be margins before the first and after the last list items. The rule .nav_item:not(:first-child) includes margin-bottom: 16px (vertical spacing) which will add a margin after the last list item and thus violates that requirement. Remove the vertical margin and keep only the horizontal spacing (use margin-left or an adjacent-sibling selector like .nav_item + .nav_item { margin-left: 30px; }).

Comment thread src/style.css
Comment on lines +35 to +38
.nav_item:not(:first-child) {
margin-left: 30px;
margin-bottom: 16px;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CSS defines a .logo class (height: 40px) but the <img> for the logo in the HTML doesn't have class="logo". As a result the logo sizing/behavior from CSS won't apply and the header won't match the Pixel Perfect requirements. Add class="logo" to the <img> (or update the CSS selector) so the logo is styled as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants