Skip to content

Develop#7736

Open
SylwiaLeung wants to merge 11 commits intomate-academy:masterfrom
SylwiaLeung:develop
Open

Develop#7736
SylwiaLeung wants to merge 11 commits intomate-academy:masterfrom
SylwiaLeung:develop

Conversation

@SylwiaLeung
Copy link
Copy Markdown

  • DEMO LINK

  • TEST REPORT LINK

  • Header height is set in 1 place (for the links)

  • Content is vertically centered (for any header height)

  • CSS is used to show all letters in Uppercase (don't type them in HTML)

  • Logo is an image wrapped with a link

  • CSS Variable is used for a blue color

  • Pseudo-element is used for a blue line below the active link

  • Code follows all the Code Style Rules ❗️

  • The Google Fonts Configuration follows requirements.
    alt text

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 requesting changes because one of the checklist requirements for the navigation links is not fully met. In src/style.css, in the .nav__link rule where you set display: flex and align-items: center, the text is vertically centered but not horizontally centered, which violates [CHECKLIST ITEM #17] Nav Links should not have any padding but have the text centered; you can fix this by adding justify-content: center to .nav__link so the link text is centered within the 60px-high clickable area. Functionally almost everything else looks solid: semantic structure, Roboto 500, flexbox header, active link pseudo-element, data-qa attributes, and hover styles are implemented correctly.

A secondary (non-blocking) issue is that some HTML lines exceed the 80-character guideline from the code style checklist; you can address this by breaking long tags (like meta and link tags) across multiple lines so each line stays within 80 characters. Another minor improvement is to move the --color-link-accent variable from inside .nav__link to a higher scope such as :root or .nav to avoid repetition and make the color easier to manage. Once you center the nav link text horizontally and optionally clean up these smaller style issues, your header implementation will fully align with the stated 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/index.html
>
<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 violates checklist item #17: "Nav Links should not have any padding but have the text centered." The .nav__link has display: flex and align-items: center (vertical centering) but no horizontal centering. Consider adding justify-content: center to .nav__link so link text is centered within the link area.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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 requesting changes because there are still code-style issues that violate the explicit checklist and one HTML formatting problem that should be corrected before this can be considered complete. In src/index.html, the <meta name="viewport" ...> tag currently has its content attribute value split across a newline, which breaks the attribute value and can lead to invalid HTML; keep the full content="width=device-width, initial-scale=1.0" string on one logical line and instead break the tag by placing each attribute on its own new line. Also in index.html, both the viewport meta tag and the long Google Fonts <link> tag exceed the 80‑character per line requirement, so please wrap them by putting each attribute on a separate line or otherwise ensure no single source line is longer than 80 characters. Similarly, in src/style.css, the line containing the long font URL goes beyond 80 characters; you can split this over multiple lines (for example, breaking after url( or before query parameters) while keeping the CSS syntactically valid. These changes will bring your code fully in line with the “Lines of code have 80 chars max” checklist rule and the expected HTML formatting, which are treated as required rather than optional style preferences. Once you adjust the line lengths and fix the attribute wrapping, the rest of your header implementation already meets the functional requirements and will be in very 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/style.css
.header__logo {
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 line contains a very long URL and likely exceeds the 80-character limit from the checklist (“Lines of code have 80 chars max”). Please wrap/break long attributes or use an alternative approach so no source line is longer than 80 characters (for example, place attributes each on their own line — or import the font from CSS with a shorter @import — so the HTML stays within the 80-char guideline).

Comment thread src/index.html Outdated
<meta
name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"
content="width=device-width, user-scalable=no,
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 is long and likely exceeds the project's 80-character limit. Please follow the checklist item: "Lines of code have 80 chars max" — break long attribute values (for example the meta viewport content) across multiple lines so each line stays within 80 chars.

Comment thread src/index.html
crossorigin="anonymous"
/>
<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 font link is a long single line; it violates: "Lines of code have 80 chars max". Split the href attribute (or put each attribute on its own line) so the line length stays under 80 characters.

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 practice these remaining HTML and style-guide details. You did a solid job on the header structure: semantic elements, flexbox layout, uppercase nav links, a proper is-active state with a ::after underline, a CSS variable for the blue color, and the data-qa="hover" attribute are all implemented correctly. The main critical issue is in src/index.html around the top of the file (roughly line 5), where the <meta name="viewport"> tag’s content value is malformed and split; it needs to be exactly content="width=device-width, initial-scale=1.0" on a single logical line to satisfy the project requirement [CHECKLIST ITEM #15]. Additionally, both the viewport <meta> line and the Google Fonts <link> line (around lines 5–15) are longer than 80 characters, so you should wrap attributes so each source line stays within 80 characters, for example by placing each attribute (rel, href, crossorigin, content, etc.) on its own line [CHECKLIST ITEM #14]. For long tags like these, keep the content string intact on one line while breaking only between attributes so that the HTML remains valid but still follows the line-length rule. As a minor improvement, double-check that your header padding matches the design and consider slightly refining the alt text for the logo if you want it to be even more descriptive. Overall, your layout and semantics are strong; tightening up these small formatting and meta-tag details will quickly bring your code fully in line with the checklist and professional HTML standards.


✨ 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
<meta
name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"
content="width=device-width, user-scalable=no, initial-scale=1 maximum-scale=1.0, minimum-scale=1.0"
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 viewport meta is incorrect and malformed. The project requires the viewport meta to keep the full string
content="width=device-width, initial-scale=1.0" (and not include extra/malformed values). Also keep this attribute
value as one logical value (don’t split it across lines). Current value includes extra parameters and a missing comma
which will produce invalid behavior. Please change it to exactly: content="width=device-width, initial-scale=1.0".
This is required by the previous correction and the task requirements.

Comment thread src/index.html
<meta
name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"
content="width=device-width, user-scalable=no, initial-scale=1 maximum-scale=1.0, minimum-scale=1.0"
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 violates the checklist rule: "Lines of code have 80 chars max". The viewport meta content line is too
long — wrap attributes/values or move the entire content attribute onto a single reasonably short line so no source
line exceeds 80 characters.

Comment thread src/style.css

html {
font-size: 12px;
line-height: 1.25;
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 previous-review requirement: "In src/index.html, the <meta name="viewport" ...> tag’s content attribute value must not be split across a newline: Keep the full string content="width=device-width, initial-scale=1.0" as one logical attribute value." The current content value is split and malformed (contains extra/misplaced parameters). Change it to content="width=device-width, initial-scale=1.0" (keep the attribute value intact on one line). You may put each attribute of the tag on its own line to keep lines ≤80 chars, but do not split the content string across lines or alter its value.

Comment thread src/style.css

.header__logo {
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 checklist item "Lines of code have 80 chars max". The Google Fonts line is too long. Wrap the long so no single source line exceeds 80 characters (for example, put each attribute on its own line or break the href into multiple lines while keeping valid HTML).

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