moyo-header #5871
moyo-header #5871
Conversation
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
Something went wrong while processing the review.
[CODE: 3]
If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
Something went wrong while processing the review.
[CODE: 3]
If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
Something went wrong while processing the review.
[CODE: 3]
If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While your code is generally well-structured, there are a few critical issues that need to be addressed before we can approve the pull request:
-
HTML Functionality: The
<a>tag wrapping the logo image lacks anhrefattribute, which is essential for making it a functional link. This is a core requirement that needs to be fixed. -
CSS Units: The
font-sizein the.navclass and theborder-radiusin the.is-active::afterselector are missing units. Adding units likepxis crucial for ensuring these styles are applied correctly. -
HTML Structure: The
<nav>element should use a list structure (<ul>and<li>) for better semantics and accessibility.
These issues impact the functionality and core requirements of the task. Please address them, and feel free to reach out if you need any help! Once these changes are made, your code will be in great shape. Keep up the great work! 🌟
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
src/index.html
Outdated
| <a> | ||
| <img | ||
| src="/src/images/logo.png" | ||
| alt="logo" | ||
| class="logo" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
The <a> tag wrapping the logo image should have an href attribute to make it a functional link. Consider adding a URL to the href attribute to direct users to the homepage or another relevant page.
| <a class="nav__link">Smartphones</a> | ||
| <a | ||
| class="nav__link" | ||
| data-qa="hover" |
There was a problem hiding this comment.
The data-qa attribute is used for testing purposes, such as identifying elements in automated tests. Ensure that this attribute is necessary for your project. If not, consider removing it to keep the HTML clean.
src/style.css
Outdated
| margin-right: 50px; | ||
| line-height: 60px; | ||
| text-transform: uppercase; | ||
| font-size: 12; |
There was a problem hiding this comment.
The font-size property is missing a unit. Consider adding a unit like px, em, or rem to specify the font size correctly.
src/style.css
Outdated
| left: 0; | ||
| bottom: 0; | ||
| height: 4px; | ||
| border-radius: 8; |
There was a problem hiding this comment.
The border-radius property should have a unit specified. Consider adding a unit like px to ensure the border radius is applied correctly.
There was a problem hiding this comment.
Great job on addressing some of the previous issues! 🎉 You've made progress by adding units to the font-size in the .nav class and the border-radius in the .is-active::after selector, which is crucial for ensuring these styles are applied correctly. However, the <nav> element still needs to be structured using a list (<ul> and <li>) for better semantics and accessibility. This is an important aspect to consider for future improvements.
The decision to approve is made to help you move forward with your learning journey. Please review the comments and address the remaining issues to enhance your code quality. Remember, learning from these experiences will help you avoid similar problems in the future. Keep up the good work and continue improving your skills! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
|
||
| .nav__link:hover { | ||
| color: var(--main-color); | ||
| } |
There was a problem hiding this comment.
The <nav> element should ideally use a list structure (<ul> and <li>) for better semantics and accessibility. Consider restructuring the navigation links within a list.
| margin-right: 50px; | ||
| line-height: 60px; | ||
| text-transform: uppercase; | ||
| font-size: 12px; |
There was a problem hiding this comment.
The font-size property in the .nav class is missing a unit. Consider adding a unit like px to ensure the style is applied correctly.
| left: 0; | ||
| bottom: 0; | ||
| height: 4px; | ||
| border-radius: 8px; |
There was a problem hiding this comment.
The border-radius property in the .is-active::after selector is missing a unit. Consider adding a unit like px to ensure the style is applied correctly.
DEMO LINK
TEST REPORT LINK
[ x] Header height is set in 1 place (for the links)
[ x] Content is vertically centered (for any header height)
[ x] CSS is used to show all letters in Uppercase (don't type them in HTML)
[ x] Logo is an image wrapped with a link
[ x] CSS Variable is used for a blue color
[ x] Pseudo-element is used for a blue line below the active link
[ x] Code follows all the Code Style Rules ❗️