-
Notifications
You must be signed in to change notification settings - Fork 6k
added header with navigation #7271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this 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 a few checklist-related issues that affect correctness rather than just style. In src/index.html, one line exceeds the 80-character style limit and the logo image uses alt="logo", which is not descriptive enough; update it to something like alt="Moyo company logo" to satisfy the accessibility and code-style requirement. In src/style.css, the selector .nav-list:first-child is incorrect for spacing list items and is redundant with your existing .nav-item + .nav-item rule, so it should be removed or replaced with a proper first-item selector (e.g., .nav-item:first-child) if you really need it. Also in src/style.css, the media query targets .nav__item while your actual class name is .nav-item; this typo breaks the responsive behavior and violates the code-style consistency requirement, so update the selector to .nav-item. Finally, your .nav-link correctly vertically centers content but does not horizontally center it; add justify-content: center (keeping padding at 0) so that the nav link text is centered inside the clickable area as required.
Once you fix these points, your solution should align well with both the functional and code-style expectations for this task.
✨ 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
| crossorigin="anonymous" | ||
| /> | ||
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line exceeds the 80-character limit specified in the code style requirements. While long URLs can sometimes be an exception, it's a good practice to be mindful of this rule.
src/index.html
Outdated
| <img | ||
| class="logo" | ||
| src="/src/images/logo.png" | ||
| alt="logo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alt attribute value "logo" is not descriptive enough. According to the requirements, it should describe the image content, for instance, alt="Moyo company logo".
src/style.css
Outdated
| .nav-list:first-child { | ||
| margin-left: 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule is incorrect and unnecessary. .nav-list:first-child selects a .nav-list that is the first child of its parent, which is not what you want here. You already create spacing between items with .nav-item + .nav-item (lines 46–47). Remove this rule or, if you intended to target the first list item, use .nav-item:first-child or .nav-list > .nav-item:first-child instead.
| .nav-link { | ||
| display: flex; | ||
| text-decoration: none; | ||
| position: relative; | ||
| align-items: center; | ||
| height: 60px; | ||
| color: black; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nav Links should not have any padding but have the text centered. Your .nav-link vertically centers text with align-items: center, but it doesn't horizontally center the link content. Add justify-content: center to .nav-link so the link text is centered inside the link area (keep padding at 0 to satisfy the requirement).
| @media (max-width: 1024px) { | ||
| .nav__item { | ||
| margin-right: 20px; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a class-name typo that breaks responsive rules and violates the checklist item: "Code follows all the Code Style Rules ❗️ (./checklist.md)" — the media query targets .nav__item but your markup and other CSS use .nav-item (hyphen). Change the selector to .nav-item so the rule applies correctly.
mateacademy-ai-mentor
left a comment
There was a problem hiding this 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 feedback from the previous review! You've fixed the major issues, and the project is very close to being complete. However, a couple of minor changes are needed to meet all the project's style requirements.
Here's what needs to be fixed:
src/index.html: The<meta name="viewport"...>tag on line 5 exceeds the 80-character limit, which violates the code style rules [CHECKLIST ITEM #7]. Please reformat this tag to place each attribute on a new line.src/style.css: The@media (max-width: 1024px)block is redundant because its style rule is already defined globally. You can safely remove this entire media query to make the code cleaner.
Once you've made these adjustments, the submission will be ready. Keep up the great work!
✨ 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
| @media (max-width: 1024px) { | ||
| .nav-item + .nav-item { | ||
| margin-left: 20px; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This media query is redundant because the rule inside it is identical to the one defined on line 42. Since it doesn't change the styling, this entire @media block can be safely removed to keep the code cleaner.
No description provided.