Skip to content

Conversation

@Starkillerr
Copy link

  • [x ] 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.

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

Good job on the overall structure using semantic HTML and flexbox. However, some changes are needed before this can be approved, as several key requirements were missed.

The most critical issue is in src/style.css. It uses a nested syntax (e.g., .nav_list inside .header) which is not valid in standard CSS and will prevent your styles from being applied by the browser. You'll need to rewrite these rules without nesting, for example: .header .nav_list { ... }.

Additionally, please address these points from the requirements:

  • The blue color #00a8ff must be defined as a CSS variable and used with var() [CHECKLIST ITEM #5].
  • The active link must use the class is-active, not active. This needs to be updated in both your HTML and CSS files.
  • The fourth navigation link is missing the required data-qa="hover" attribute.

Please make these corrections, and I'll be happy to review your work again!


✨ 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
Comment on lines 20 to 64
.nav_list {
display: flex;
margin: 0;
padding: 0;
align-items: center;
list-style: none;
}
.nav_item {
margin-right: 30px;
}
.nav_item:last-child {
margin-right: 0;
}
.nav_link {
display: flex;
text-decoration: none;
color: #000;
height: 60px;
text-transform: uppercase;
position: relative;
align-items: center;
}
.logo {
height: 40px;
width: 40px;
}
.nav_link:hover {
color: #00a8ff;
}
.nav_link.active {
color: #00a8ff;
}

.nav_link.nav_link.active::after {
content: '';
height: 2px;
width: 100%;
background-color: #00a8ff;
border-bottom: 3.5px solid #00a8ff;
border-radius: 10px;
position: absolute;
left: 0;
right: 0;
bottom: 0;
}

Choose a reason for hiding this comment

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

This file uses a nested syntax which is not valid in a standard CSS file. This syntax is common in preprocessors like SASS/SCSS, but a browser cannot parse it directly. You need to rewrite these rules without nesting. For example, .nav_list should become a top-level selector like .header .nav_list.

src/style.css Outdated
width: 40px;
}
.nav_link:hover {
color: #00a8ff;

Choose a reason for hiding this comment

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

This violates checklist item #5: 'CSS Variable is used for a blue color'. You should define the color #00a8ff as a CSS variable and use it with var() here and in other places. This improves maintainability.

src/style.css Outdated
.nav_link:hover {
color: #00a8ff;
}
.nav_link.active {

Choose a reason for hiding this comment

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

The task requirements specify that the active link should have the class is-active. Your selector uses .active. Please update this to match the requirements.

src/style.css Outdated
color: #00a8ff;
}

.nav_link.nav_link.active::after {

Choose a reason for hiding this comment

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

The selector .nav_link.nav_link.active::after is unnecessarily specific because the .nav_link class is repeated. Using .nav_link.active::after is sufficient and more readable.

src/style.css Outdated
Comment on lines 55 to 58
height: 2px;
width: 100%;
background-color: #00a8ff;
border-bottom: 3.5px solid #00a8ff;

Choose a reason for hiding this comment

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

The implementation of the underline has a couple of issues:

  1. The border-bottom uses a fractional value (3.5px), which goes against the requirement to avoid uneven sizes.
  2. Using height, background-color, and border-bottom together is redundant. A cleaner way to create the line is to use only height and background-color (e.g., height: 4px;).

src/index.html Outdated
<ul class="nav_list">
<li class="nav_item">
<a
class="nav_link active"

Choose a reason for hiding this comment

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

The requirement is to use the class name is-active for the active link, but active is used here. Please update the class name to match the specification.

Copy link
Author

@Starkillerr Starkillerr left a comment

Choose a reason for hiding this comment

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

  • 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.

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 progress on this task! You've done a nice job refactoring the CSS and using variables. However, changes are needed because the automated checks are failing due to two main issues.

First, the tests can't find a required element. Please add the data-qa="hover" attribute to the 4th navigation link (Laptops & Computers) in src/index.html to resolve the test failures.

Second, there are visual mismatches with the design. To fix this, please review the Figma mockup and add the missing typography styles, such as font-size and letter-spacing, to your .nav_link rule in src/style.css.

Once these are fixed, we should be good to go!


✨ 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

html {
box-sizing: border-box;

--primary-blue: #00a0ff;

Choose a reason for hiding this comment

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

For global variables like this, it's a common convention to define them within the :root pseudo-class instead of the html selector. While both work in this case, :root is generally preferred as it has higher specificity and clearly signals that these are global variables for the entire document.

Comment on lines 62 to 67
<a
class="nav_link"
href="#"
>
Laptops & Computers
</a>

Choose a reason for hiding this comment

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

This link is missing the required data-qa="hover" attribute. The task description specifies: 'add data-qa="hover" attribute to the 4th link for testing (Laptops & computers)'.

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 job addressing the previous feedback and getting the HTML structure just right! I am approving this submission as an educational override so you can move forward, but there are a few key CSS adjustments needed to achieve that pixel-perfect match with the Figma design.

The main issue is with the navigation links in src/style.css. They are currently missing the correct font-size, and the height is too small. Remember, the link's height should define the entire header's height, which will also fix the vertical centering of the text. Once you adjust the height, you'll also need to recalculate the top position for the active link's underline to place it correctly beneath the text.

Keep up the great work, and pay close attention to these details in future tasks!


✨ 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 +39 to +49
.nav_link {
display: flex;
text-decoration: none;
color: #000;
height: 15px;
letter-spacing: 0%;

text-transform: uppercase;
position: relative;
align-items: center;
}

Choose a reason for hiding this comment

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

This rule is missing key typography styles needed to match the Figma mockup, which was pointed out in the previous review.

  1. The font-size property is missing. Please add it based on the design specifications.
  2. The height: 15px is incorrect for setting the header's height. This violates checklist item Task #1: 'Header height is set in 1 place (for the links)'. The link's height should define the entire header's height (e.g., 80px). Adjusting this will also correctly center the link text vertically.

background-color: var(--primary-blue);

border-radius: 8px;
top: 30px;

Choose a reason for hiding this comment

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

This top value will likely position the underline too far from the text once the parent link's height is corrected. The line should appear just below the text. You will need to recalculate this value to match the mockup.

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