style: fix header layout and spacing#7477
style: fix header layout and spacing#7477frolova11 wants to merge 14 commits intomate-academy:masterfrom
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good job on the overall structure and use of flexbox. However, several changes are needed to meet the project's core requirements. Your solution currently violates a few key constraints which causes parts of the styling and functionality to break.
Here are the main issues to address:
- The
gapproperty is used insrc/style.css, but the task explicitly forbids it. Please usemarginon the list items for spacing instead. - The Google Fonts link in
src/index.htmlimports all font weights, but the requirement is to import only the medium (500) weight. [CHECKLIST ITEM #8] - The navigation links are styled using a tag selector (
.nav a), which is not allowed. Please add a class like.nav__linkto the<a>tags and use that for styling. This will also fix the broken:hovereffect. - The active link's blue line is not appearing due to a typo in a CSS variable; you've used
--blueinstead of the declared--color-blue.
Please address these points. You're on the right track!
✨ 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/index.html
Outdated
| rel="stylesheet" | ||
| href="./style.css" | ||
| /> | ||
| <link href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100..900;1,100..900&display=swap" rel="stylesheet"> |
There was a problem hiding this comment.
This font link violates the requirements. You should only import the Roboto font with roman style and medium (500) weight. The current link imports all weights from 100 to 900 for both roman and italic styles, which is not what was requested in the task description. This violates checklist item #8.
src/index.html
Outdated
| <li><a href="#Apple" class="is-active">Apple</a></li> | ||
| <li><a href="#Samsung">Samsung</a></li> | ||
| <li><a href="#Smartphones">Smartphones</a></li> | ||
| <li><a href="#Laptops & Computers" data-qa="hover">Laptops & Computers</a></li> | ||
| <li><a href="#Gadgets">Gadgets</a></li> | ||
| <li><a href="#Tablets">Tablets</a></li> | ||
| <li><a href="#Photo">Photo</a></li> | ||
| <li><a href="#Video">Video</a></li> |
There was a problem hiding this comment.
The list items (<li>) and anchor tags (<a>) inside the navigation are missing their BEM classes. According to your style.css and the project's code style rules, they should have classes like nav__item and nav__link respectively. Without these, the styles for links, including hover effects and margins, will not apply correctly.
src/index.html
Outdated
| <li><a href="#Apple" class="is-active">Apple</a></li> | ||
| <li><a href="#Samsung">Samsung</a></li> | ||
| <li><a href="#Smartphones">Smartphones</a></li> | ||
| <li><a href="#Laptops & Computers" data-qa="hover">Laptops & Computers</a></li> |
There was a problem hiding this comment.
The href attribute should not contain spaces. While browsers might handle this, it's not a valid value for a URL fragment. Consider using a hyphen or removing the space, like #Laptops-Computers.
src/style.css
Outdated
| :root { | ||
| --color-blue: #00acdc; | ||
| } | ||
| .body { |
There was a problem hiding this comment.
The body tag is one of the few exceptions where a tag selector is allowed. This .body class selector is currently not being applied because the <body> element in your HTML file does not have class="body".
src/style.css
Outdated
| justify-content: space-between; | ||
| align-items: center; | ||
| } | ||
| .nav a { |
There was a problem hiding this comment.
This selector violates the requirement: "Don't use tag names for styling (except html and body)". Please use a class for the navigation links, for example .nav__link, and apply it to the <a> tags in your HTML. This will also allow your :hover styles on line 29 to work correctly.
| line-height: 60px; | ||
| } | ||
|
|
||
| .nav__link:hover { |
There was a problem hiding this comment.
This selector for the hover effect is not working because the <a> elements in the HTML are missing the nav__link class. The same issue applies to the .nav__link.is-active selector on line 43.
src/style.css
Outdated
| } | ||
|
|
||
| .nav__link:hover { | ||
| color: var(--blue, #00acdc); |
There was a problem hiding this comment.
There's a typo in the CSS variable name. You've defined it as --color-blue on line 2, but you're referencing --blue here. The same mistake is present on line 48.
src/style.css
Outdated
| list-style: none; | ||
| display: flex; | ||
| margin: 0; | ||
| gap: 20px; |
There was a problem hiding this comment.
The task requirements explicitly forbid using the gap property. Please use margin on the list items instead to create the space between them.
src/style.css
Outdated
| .nav.navbar { | ||
| text-transform: uppercase; | ||
| } | ||
| .nav__item:not(:last-child) { | ||
| margin-right: 20px; |
There was a problem hiding this comment.
These CSS rules are ineffective because the corresponding classes (.nav__item and .navbar) are missing from your HTML file. Ensure your CSS selectors match the classes used in your markup.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed almost all the feedback from the previous review, and the project is very close to perfect. However, a couple of small changes are needed before this can be approved.
- In
src/index.html, thehrefattribute for one of the links contains a space ("#Laptops & Computers"). URL fragments should not contain spaces to ensure they work correctly. Please replace the space with a hyphen, like"#laptops-and-computers". - In
src/style.css, there are two unused CSS rules:.logo(the class in the HTML isheader__logo) and an incorrectly structured selector.nav__list.nav__item.... Please remove these to keep the stylesheet clean.
After these minor adjustments, your work will be ready for approval. Keep up the great effort!
✨ 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
| .nav__list.nav__item:not(:last-child).nav__link { | ||
| margin-right: 20px; | ||
| } |
There was a problem hiding this comment.
This CSS selector is not structured correctly for the HTML provided. It tries to match a single element that has .nav__list, .nav__item, and .nav__link classes at the same time, which is not possible as these are separate elements (ul, li, a). This rule currently has no effect. The correct rule for spacing is already present on line 66.
src/style.css
Outdated
| .logo { | ||
| display: block; | ||
| } |
There was a problem hiding this comment.
This style rule targets a class named .logo. However, in your index.html file, the logo's anchor tag has the class header__logo. Because the class names don't match, these styles are not being applied.
src/index.html
Outdated
| <li class="nav__item"><a class="nav__link is-active" href="#Apple">Apple</a></li> | ||
| <li class="nav__item"><a class="nav__link" href="#Samsung">Samsung</a></li> | ||
| <li class="nav__item"><a class="nav__link" href="#Smartphones">Smartphones</a></li> | ||
| <li class="nav__item"><a class="nav__link" href="#Laptops & Computers" data-qa="hover">Laptops & Computers</a></li> |
There was a problem hiding this comment.
The href attribute value should not contain spaces. This can cause issues with URL encoding and fragment identifiers (the part after the #). It's best practice to use hyphens or underscores instead of spaces in href values.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! You've successfully addressed all the feedback from the previous review, and your solution now fully meets all project requirements. Your HTML is well-structured with correct semantic tags, and the CSS is clean and efficient. This is a great submission, and I am approving it. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.