Skip to content

Conversation

@Staskovych
Copy link

Copy link

@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

This is a strong attempt, and you've implemented the flexbox layout correctly. However, a couple of changes are needed to meet the project's core requirements before this can be approved.

First, the task explicitly forbids using tag selectors for styling (except for html and body). Your CSS currently styles the <header> tag directly. Please add a class to the header in your HTML and use that class selector for all its styles.

Second, the header's height should be determined by the line-height of the navigation links, as stated in [CHECKLIST ITEM #1]. Currently, a fixed height is applied to the header container itself. Please remove this and set the height via the links.

Once these two issues are addressed, your solution will be ready for approval. You're very close!


✨ 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

src/style.css Outdated
font-weight: 500;
}

header {

Choose a reason for hiding this comment

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

The requirements state: 'Don't use tag names for styling (except html and body)'. Please add a class to the <header> element in your HTML and use that class for styling.

src/style.css Outdated
box-sizing: border-box;
align-items: center;
padding: 0 50px;
height: 60px;

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)'. The height should be defined by the line-height of the navigation links, not by an explicit height on the header container.

src/style.css Outdated
}

@media (max-width: 1024px) {
header {

Choose a reason for hiding this comment

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

This also violates the rule against using tag selectors. Please use the class you add to the header for this media query rule as well.

src/index.html Outdated
/>
<title>Moyo header</title>
<link
href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap"

Choose a reason for hiding this comment

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

This line slightly exceeds the 80-character limit. Please ensure all lines of code respect the maximum length as per the project's style guide.

src/index.html Outdated
>
<img
src="./images/logo.png"
alt="moyo-logo"

Choose a reason for hiding this comment

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

While moyo-logo is acceptable, a more descriptive alt text like "Moyo company logo" would be better for accessibility, as it provides more context for screen reader users.

Copy link

@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

Great work on this iteration! You've successfully addressed most of the previous feedback, and the solution is very close to being complete. However, one critical issue remains that needs to be fixed.

In your CSS, the @media (max-width: 1024px) block re-introduces a fixed height on the .header. This contradicts a core requirement that the header's height must be determined only by the line-height of the navigation links ([CHECKLIST ITEM #1]). Additionally, the width: 1024px in that same rule will cause unwanted horizontal scrolling on smaller screens.

Please remove both the height and width properties from the .header within that media query. Once that's adjusted, you'll be all set!


✨ 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

src/style.css Outdated
@media (max-width: 1024px) {
.header {
width: 1024px;
height: 60px;

Choose a reason for hiding this comment

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

This line violates the requirement that the header's height should be determined by the navigation links, not a fixed height on the header element itself. This is mentioned in checklist item #1: 'Header height is set in 1 place (for the links)'.

Please remove this height property. The line-height on the .nav__link already establishes the header's height.

Comment on lines +80 to +83
class="nav__link"
>
Tablets
</a>

Choose a reason for hiding this comment

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

This media query reintroduces an issue from the previous review. Setting a fixed height on the .header violates the requirement that the header's height should be determined by the line-height of the links ([CHECKLIST ITEM #1]).

Additionally, width: 1024px will prevent the header from stretching to the full page width on smaller screens, causing horizontal scrolling.

Copy link

@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

Excellent work on this update! You've successfully fixed the main layout issue from the previous review. I'm approving this solution as an educational override so you can keep moving forward, but there are a few minor points to consider for future projects.

Here are a couple of small improvements:

  • The navigation link text needs to be centered. You can achieve this by adding text-align: center; to the .nav__link rule in style.css.
  • There's a redundant @media block at the end of style.css that can be removed, as its style is already defined elsewhere.

Overall, this is a fantastic submission and very close to perfect. Keep up the great work!


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

Footnotes

  1. Rate AI review example

Comment on lines +72 to +75
@media (max-width: 1024px) {
.nav__item {
margin-left: 20px;
}

Choose a reason for hiding this comment

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

This @media block contains a rule for .nav__item that is identical to the one defined on line 34. Since it provides no new styling information, this entire block is redundant and can be removed.

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