solution #7273
solution #7273
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical checklist requirements and test blockers that are not yet met. First, in src/index.html you need to fix the image path separator (use / instead of \), improve the alt text to something descriptive like Moyo website logo, and correct the typo in the active link class so it is exactly class="nav__link is-active"; these affect accessibility and the selectors the tests rely on. Second, the 4th navigation link must have data-qa="hover" so the end-to-end tests can find and interact with it. Third, in src/style.css you must properly embed the required Google Font variant (Roboto, roman style, weight 500 only) via the Google Fonts <link> or @import, because this is an explicit task requirement and a cause of visual test failures. Finally, clean up the ineffective align-items: center; on .nav__link, since it is not a flex container and this can confuse future maintenance; vertical centering is already handled via line-height. Once you make these fixes, your solution should align with the specification and your existing good work on flexbox, variables, and pseudo-elements will pass the automated checks.
✨ 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
| class="logo" | ||
| > | ||
| <img | ||
| src="images\logo.png" |
There was a problem hiding this comment.
The file path for the image uses a backslash (\). For web URLs, you should always use a forward slash (/) as a path separator to ensure it works across all operating systems and servers.
src/index.html
Outdated
| > | ||
| <img | ||
| src="images\logo.png" | ||
| alt="logo" |
There was a problem hiding this comment.
The alt attribute value logo is a bit generic. The requirement is to describe the image content. A more descriptive value like "Moyo website logo" would be better for accessibility.
src/index.html
Outdated
| <li class="nav__item"> | ||
| <a | ||
| href="1" | ||
| class="nav__link is is-active" |
There was a problem hiding this comment.
The class is seems to be a typo here. While it doesn't break the styling because of the .is-active selector, it's an extra, unused class. It should be removed to have class="nav__link is-active".
| @@ -1,3 +1,79 @@ | |||
| html { | |||
| font-family: Roboto, sans-serif; | |||
There was a problem hiding this comment.
You set font-family: Roboto here, but the task requires adding the Google Fonts configuration that selects ONLY the roman style, medium (500) weight and normal width. This violates checklist item: "The Google Fonts Configuration follows requirements (as shown in the referenced image)." Add the appropriate <link> (or @import) that loads only the required Roboto variant so auto-tests pass.
src/style.css
Outdated
| display: block; | ||
| padding: 0; | ||
| line-height: 60px; | ||
| align-items: center; |
There was a problem hiding this comment.
align-items: center; on .nav__link has no effect because .nav__link is display: block (not a flex container). Remove this property or make the link a flex container if you intended to use flex alignment. This is a functional cleanup — vertical centering is already handled by line-height: 60px.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've addressed almost all the feedback, and the solution is very close to being perfect. There is just one small detail missing in src/style.css. According to the requirements, the navigation link text should be centered, but it is currently aligned to the left. You can easily fix this by adding the appropriate text-align property to your nav link's style rule. Once that's fixed, the implementation will be complete. You're almost there!
✨ 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
| .nav__link { | ||
| text-decoration: none; | ||
| color: black; | ||
| display: block; | ||
| padding: 0; | ||
| line-height: 60px; | ||
| } |
There was a problem hiding this comment.
The implementation is almost perfect, but it's missing one detail from the requirements: "Nav Links should not have any padding but have the text centered". The text inside the links is currently aligned to the left. You can fix this by adding the appropriate text alignment property to this rule.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! I'm happy to approve your solution as you've successfully addressed the previous feedback and the navigation text is now correctly centered.
This is an educational approval to help you progress, and I have a couple of minor suggestions for writing even cleaner code in the future. In src/style.css, you can safely remove the display: inline-block from the .nav element, as it has no effect inside a flex container. Additionally, the .nav__item:last-child rule is not needed because you are only applying margin-left to the items.
These are just small refinements, and your overall implementation is very strong. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
|
|
||
| .nav { | ||
| display: inline-block; |
There was a problem hiding this comment.
Since the .header is a flex container, this .nav element is a flex item. Flex items behave like block-level elements, so display: inline-block has no effect here. This line can be safely removed.
| .nav__item:last-child { | ||
| margin-right: 0; | ||
| } |
There was a problem hiding this comment.
This rule for :last-child is not necessary. You are creating space between items using margin-left, and you correctly remove it from the first item. Since no margin-right is ever applied, there's no need to remove it from the last item.
2 participants
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.
