Skip to content

Moyo header #7739

Open
mariiaherasymiukdev-bit wants to merge 3 commits intomate-academy:masterfrom
mariiaherasymiukdev-bit:develop
Open

Moyo header #7739
mariiaherasymiukdev-bit wants to merge 3 commits intomate-academy:masterfrom
mariiaherasymiukdev-bit:develop

Conversation

@mariiaherasymiukdev-bit
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

Changes are needed before this task can be considered complete because a few checklist-based requirements are still violated.

  1. In src/style.css, the selector .logo img uses the img tag name for styling, which conflicts with [CHECKLIST ITEM #41] (“Don’t use tag names for styling (except html and body)”). Add a class to the logo image in src/index.html (e.g. <img class="logo__img" ...>), and update the CSS to target .logo__img instead of .logo img.
  2. In src/index.html, the Google Fonts link for Roboto should explicitly request the roman (non-italic) axis with weight 500 as required by [CHECKLIST ITEM #4]. Replace the current href with one that uses the ital,wght@0,500 style configuration so that only roman 500 is requested.
  3. Your HTML is missing blank lines between multi-line sibling blocks (for example, between separate <li> blocks), which the checklist requires for readability. Add an empty line between each multi-line sibling element to satisfy that formatting rule.
  4. The assignment also expects the PR description to include the full checklist with checked boxes; be sure to copy the checklist into your PR body and mark completed items with - [x].

Functionally your header implementation looks solid (flexbox layout, active link styling, hover states, etc.), so once you address these checklist-related issues, your solution should meet all the requirements.


✨ 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/style.css Outdated
Comment on lines +24 to +26
.logo img {
display: block;
height: 40px;
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 #41: "Don't use tag names for styling (except html and body)". The selector .logo img targets the img tag; prefer adding a class to the image (for example .logo__image) and style the image via that class to avoid using a tag selector.

Comment thread src/index.html Outdated
/>
<title>Moyo header</title>
<link
href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap"
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 likely violates the checklist requirement: "[CHECKLIST ITEM #4] Use Roboto font from Google Fonts: select ONLY roman style, medium (500) weight and normal width for embedding; the Google Fonts configuration must follow these requirements." The current href (loaded as family=Roboto:wght@500&display=swap) may not explicitly request the roman (non-italic) axis expected by the tests. Please use the Google Fonts config that requests roman only and weight 500 (for example include the ital,wght@0,500 pattern) so the font embedding matches the checklist exactly.

Comment thread src/index.html
</head>
<body>
<h1>Moyo header</h1>
<header class="header">
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 #41] Don't use tag names for styling (except html and body)." The selector .logo img uses the img tag name. Instead add a class to the image (for example .logo__img) and style that class (e.g. .logo__img { display:block; height:40px; }) so styling is done via class selectors only.

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

Changes are needed because a few explicit checklist and requirement items are still not satisfied, even though the header is otherwise implemented well. In src/index.html, the multi-line <li> elements inside the navigation (and any other multi-line sibling HTML blocks) are written directly one after another without a blank line, which violates the rule to add empty lines between multi-line sibling blocks; insert an empty line between each multi-line <li> block to match the checklist’s GOOD example. Also in src/index.html, at least the Google Fonts <link> and the viewport <meta> tag exceed 80 characters on a single line, so you should split attributes/values across multiple lines so every line stays within the 80‑character limit. In your CSS, .nav__link sets height and line-height but doesn’t explicitly horizontally center the text, so add text-align: center; (or an equivalent approach) to .nav__link to fully meet the requirement that nav link text is centered without padding. The assignment instructions also require that you copy the provided Checklist into the Pull Request description and mark completed items with - [x]; make sure you add that checklist after the DEMO and TEST REPORT links so the PR metadata complies with the task. As a minor improvement, consider changing alt="MoyoLogo" to a more readable alt="Moyo logo" for better accessibility. Once these formatting and metadata issues are addressed, your solution will fully align with both the functional requirements and the checklist expectations.


✨ 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/style.css
Comment on lines +38 to +46
margin: 0;
}

.nav__item:not(:last-child) {
margin-right: 20px;
}

.nav__link {
display: block;
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 the checklist item: "Add empty lines between multiline sibling blocks of HTML". Many of your <li> elements are written as consecutive multi-line sibling blocks without blank lines between them (this matches the BAD example in the checklist). Please add an empty line between each multi-line sibling element (for example between each multi-line <li> block) so the structure matches the GOOD example.

Comment thread src/index.html
Comment on lines +37 to +103
<ul class="nav__list">
<li class="nav__item">
<a
href="#"
class="nav__link is-active"
>
apple
</a>
</li>
<li class="nav__item">
<a
href="#"
class="nav__link"
>
samsung
</a>
</li>
<li class="nav__item">
<a
href="#"
class="nav__link"
>
smartphones
</a>
</li>
<li class="nav__item">
<a
href="#"
class="nav__link"
data-qa="hover"
>
laptops & computers
</a>
</li>
<li class="nav__item">
<a
href="#"
class="nav__link"
>
gadgets
</a>
</li>
<li class="nav__item">
<a
href="#"
class="nav__link"
>
tablets
</a>
</li>
<li class="nav__item">
<a
href="#"
class="nav__link"
>
photo
</a>
</li>
<li class="nav__item">
<a
href="#"
class="nav__link"
>
video
</a>
</li>
</ul>
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: "Add empty lines between multiline sibling blocks of HTML" — the

  • elements inside the navigation are multi-line sibling blocks but there are no empty lines between them. Add a blank line between each multi-line
  • block (see the checklist "GOOD example").

  • Comment thread src/index.html Outdated
    />
    <title>Moyo header</title>
    <link
    href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,500&display=swap"
    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: "Lines of code have 80 chars max" — the Google Fonts href URL on this line is longer than 80 characters. Place attributes/values on separate lines or otherwise ensure the line length stays within 80 characters.

    Comment thread src/index.html
    Comment on lines +45 to +54
    </li>
    <li class="nav__item">
    <a
    href="#"
    class="nav__link"
    >
    samsung
    </a>
    </li>
    <li class="nav__item">
    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 requirement: "Nav Links should not have any padding but have the text centered" — the CSS for .nav__link sets height and line-height but does not horizontally center the link text. Add horizontal centering (for example text-align: center;) to .nav__link so the text is centered inside each link.

    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 approving your solution as an educational override so you can keep progressing while you address a few remaining checklist issues. Your header structure, flexbox layout, active-link styling, and general CSS organization look solid and meet the core functional requirements.

    Critical things to fix next:

    • In src/index.html (around the <head> section, e.g., lines 8–15) and any Google Fonts usage in src/style.css, the Roboto import currently requests italic variants via ital,wght@0,500;1,500; it must request only the roman medium weight, e.g. ?family=Roboto:wght@500&display=swap [CHECKLIST ITEM #17].
    • In src/index.html, the meta name="viewport" content attribute and the Google Fonts <link> href line are longer than 80 characters; split attributes across multiple lines so no line exceeds 80 characters, keeping one attribute per line and indenting consistently [CHECKLIST ITEM #38, CHECKLIST ITEM #46].
    • In your Pull Request description, remember to paste the full checklist and mark items as - [x] after the DEMO and TEST REPORT links so automated checks can verify your self-review [CHECKLIST ITEM #14].

    Overall, you did well on the layout and semantics; once you tighten up the font configuration, line length, and PR checklist, your submission will be fully aligned with the requirements—nice work so far, and keep refining these details.


    ✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

    Footnotes

    1. Rate AI review example

    Comment thread src/index.html
    />
    <title>Moyo header</title>
    <link
    href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,500;1,500&display=swap"
    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 #17: "use Roboto font: select ONLY roman style, medium (500) weight and normal width for embedding." The current Google Fonts href requests italic as well (it contains ital,wght@0,500;1,500). Also this line appears to exceed the 80-character limit — see checklist item #46 below. Please load only the roman 500 variant (for example: ?family=Roboto:wght@500&display=swap) and ensure the long href is wrapped/split so no HTML line exceeds 80 characters.

    Comment thread src/style.css
    .header {
    display: flex;
    align-items: center;
    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 line includes a Google Fonts request that doesn't follow the task requirements. It violates the checklist item: "The Google font Roboto must be used, embedding only roman style, medium (500) weight, and normal width." The current href requests italic (1,500) as well. Also this attribute value is a long line exceeding the 80-char limit which violates: "Lines of code have 80 chars max". Please change the Google Fonts URL to request only the roman 500 weight (for example use a family query that requests only 0,500 / wght@500) and reformat the tag so no single line is longer than 80 characters.

    Comment thread src/style.css
    font-weight: 500;
    font-size: 12px;
    }

    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 line exceeds the allowed maximum line length. It violates the checklist item: "Lines of code have 80 chars max". Split long attribute values (like the viewport content) across multiple lines following the checklist formatting rules so every line stays within 80 characters.

    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