Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good work on the semantic HTML structure and the use of Flexbox! However, this submission requires a few changes to meet the core requirements. The main issue is that the implementation is not 'Pixel Perfect' because the header's height is incorrect. To fix this, you must set a specific height on the navigation links (<a> tags) to control the header's dimensions as specified in the design. Additionally, the project rules disallow styling HTML tags directly. Please add classes to the <header> and <li> elements and update your CSS to use class selectors instead. Addressing these points will ensure your solution fully meets the task 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
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this submission! You've successfully addressed most of the previous feedback. However, changes are needed because the build is failing and the design doesn't yet match the "Pixel Perfect" requirement.
Here are the critical issues to fix:
- Failing Build: The build is failing due to a few linting errors in
src/index.html. Please add the required<title>tag, provide a value for thecrossoriginattribute on the<link>tag, and set the logo'shrefto#. - Container Width: To match the design, the container's
max-widthinsrc/style.cssshould be a fixed1140pxas specified in the mockup, not a percentage. - Underline Position: The active link's underline is positioned outside the header due to
bottom: -8px. Please adjust this to a positive value to place it correctly within the link's area.
Additionally, for better code quality, please remove the invalid width: normal; property and merge the duplicated .nav_list styles into a single block. Addressing the critical points will get your solution ready for approval!
✨ 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
src/style.css
Outdated
| font-family: 'Roboto', Arial, sans-serif; | ||
| font-style: normal; | ||
| font-weight: 500; | ||
| width: normal; |
There was a problem hiding this comment.
The value normal is not valid for the width property in CSS. This property should be removed as it has no effect and is incorrect.
src/style.css
Outdated
| background-color: #ffffff; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| max-width: 80%; |
There was a problem hiding this comment.
The requirement is to match the design 'Pixel Perfect'. The Figma mockup specifies a content container width of 1140px. Using a percentage-based max-width will not match the design on all screen sizes. Please use a fixed value.
src/style.css
Outdated
| .nav_list { | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
The styles for the .nav_list selector are split into two separate rule blocks. It's best practice to define all styles for a selector in one place for better readability and maintenance. Please merge this display: flex; property into the .nav_list block on line 27.
src/style.css
Outdated
| width: 100%; | ||
| height: 2px; | ||
| background-color: var(--color-primary); | ||
| bottom: -8px; |
There was a problem hiding this comment.
A negative bottom value places the underline outside of the link's clickable area, which doesn't match the visual design in Figma. The underline should be positioned 8px below the text but inside the 56px height of the link. You'll need to calculate a positive bottom value to achieve this.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution so you can keep moving forward, but please take a moment to review the feedback below.
Excellent work addressing the previous comments! The HTML structure is solid, and you've implemented most of the design requirements correctly. The main reason the build is failing is a visual mismatch due to a missing font-size on the navigation links in src/style.css. Adding the correct font size from the design file will fix this and make your solution pixel-perfect.
I've also left a few minor suggestions in the code comments to improve code style in src/index.html and reduce selector specificity in src/style.css. Addressing these will make your code even better. You're doing great, keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
src/index.html
Outdated
| <li class="nav_items"><a class="nav_links is-active" href="#">Apple</a></li> | ||
| <li class="nav_items"><a class="nav_links" href="#">Samsung</a></li> | ||
| <li class="nav_items"><a class="nav_links" href="#">Smartphones</a></li> | ||
| <li class="nav_items"><a class="nav_links" data-qa="hover" href="#">Laptops & Computers</a></li> |
There was a problem hiding this comment.
This line is longer than 80 characters. The <a> element also has more than two attributes, so according to the code style rules, it should be formatted with each attribute on a new line. Applying this formatting will also fix the line length issue.
src/style.css
Outdated
| .nav_list .nav_links { | ||
| display: inline-block; | ||
| text-transform: uppercase; | ||
| } | ||
|
|
||
| .logo { | ||
| display: flex; | ||
| } | ||
|
|
||
| .nav_list .nav_links:link { | ||
| text-decoration: none; | ||
| color: black; | ||
| } | ||
|
|
||
| .nav_list .nav_links:visited { | ||
| text-decoration: none; | ||
| color: black; | ||
| } | ||
|
|
||
| .nav_list .nav_links:hover { | ||
| color: var(--color-primary); | ||
| } | ||
|
|
||
| .nav_list .nav_links.is-active { | ||
| position: relative; | ||
| color: var(--color-primary); | ||
| } | ||
|
|
||
| .nav_list .nav_links.is-active::after { | ||
| content: ""; | ||
| position: absolute; | ||
| width: 100%; | ||
| height: 2px; | ||
| background-color: var(--color-primary); | ||
| bottom: 8px; | ||
| left: 0; | ||
| border-radius: 8px; | ||
| } |
There was a problem hiding this comment.
These selectors are more specific than necessary due to the .nav_list prefix. This can make styles harder to override and less reusable.
You can improve this by:
- Consolidating the common link styles (from the rules on lines 43-46, 52-55, and 57-60) into the main
.nav_linksrule (lines 38-41). - Removing the
.nav_listprefix from all these selectors (e.g., use.nav_links:hoverinstead of.nav_list .nav_links:hover).
This will make your CSS cleaner and more maintainable.
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.
