added homework getting files after checklist accomplishing and errors… #7275
Conversation
… in script tests result
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've done a great job setting up the HTML structure and using flexbox. However, changes are needed because a critical requirement is not met. The Google Fonts URL in index_moyo_header_error.html is incorrect (wght@0500 should be wght@500), which prevents the required Roboto font from loading [CHECKLIST ITEM #8]. Please correct this to ensure the page uses the correct font. Additionally, to match the design perfectly, the hover effect on navigation links should only change the color, not add an underline. Lastly, the alt text for the logo should be more descriptive for accessibility, such as 'Moyo website logo'. After you've addressed these points, your solution will be 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
| @media (min-width: 1024px) { | ||
| .nav { | ||
| padding-left: 333px; | ||
| padding-right: 20px; | ||
| } | ||
| } | ||
|
|
||
| @media (min-width: 1200px) { | ||
| .nav { | ||
| padding-left: 505px; | ||
| padding-right: 20px; | ||
| } |
There was a problem hiding this comment.
Using hardcoded padding-left values within media queries to position the navigation is not a robust solution. This will easily break if the viewport width is different or if the content changes. A better approach would be to let flexbox handle the alignment. For example, you could remove justify-content: space-between from the .header and add margin-left: auto; to the .nav selector to push it to the right.
| .nav__link { | ||
| text-decoration: none; | ||
| color: #000; | ||
| font-size: 12px; | ||
| text-transform: uppercase; | ||
| display: flex; | ||
| align-items: center; | ||
| height: 60px; | ||
| position: relative; | ||
| } |
There was a problem hiding this comment.
The requirement states that 'Nav Links should have their text centered'. While align-items: center handles vertical centering, the text is not horizontally centered. You should add justify-content: center to this rule to center the text horizontally within the link area.
|
|
||
| .nav__link:hover { | ||
| color: var(--secondary-color); | ||
| text-decoration: underline; |
There was a problem hiding this comment.
The task requires the implementation to be 'Pixel Perfect' according to the Figma mockup. The design shows that the link's color changes on hover, but it does not become underlined. Please remove this text-decoration property to match the design.
| .blue { | ||
| color: var(--secondary-color); | ||
| } |
There was a problem hiding this comment.
This .blue class is redundant because the color is already being set by the .nav__link.is-active selector. Additionally, class names should represent the content's meaning, not its style (e.g., 'blue'). It's best to remove this class from both the CSS and the HTML.
| <link rel="stylesheet" href="normalize.css"> | ||
| <link rel="preconnect" href="https://fonts.googleapis.com"> | ||
| <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin> | ||
| <link href="https://fonts.googleapis.com/css2?family=Roboto:wght@0500&display=swap" rel="stylesheet"> |
There was a problem hiding this comment.
This line has a few issues. First, the font weight in the URL, wght@0500, is incorrect. It should be wght@500 to match the requirement for 'medium (500) weight'. This violates checklist item #8. Second, according to the code style guide, when a tag has more than two attributes, each attribute should be on a new line. This also helps with the line length, which currently exceeds 80 characters.
| <body> | ||
| <header class="header"> | ||
| <a class="logo__img" href="logo.html"> | ||
| <img src="moyo_1.png" alt="logotype" /> |
There was a problem hiding this comment.
The alt text 'logotype' is too generic. Per the task checklist, it's better to be more descriptive for accessibility, for example, 'Moyo website logo'.
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a class="nav__link blue is-active" href="#">Apple</a> |
There was a problem hiding this comment.
The blue class here is redundant. The is-active class should be sufficient to apply all styles for the active state, including the color. Using class names that describe appearance (like 'blue') is also discouraged by the style guide. Lastly, please format attributes on new lines as this tag has more than two.
2 participants
… in script tests result