Develop #5867
Develop #5867
Conversation
PrettyWisee
commented
Dec 26, 2024
PrettyWisee
commented
- [DEMO LINK] https://PrettyWisee.github.io/layout_moyo-header/
- [TEST REPORT LINK] https://PrettyWisee.github.io/layout_moyo-header/report/html_report/
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 However, there are a few critical issues that need to be addressed before we can approve the pull request. The use of the * selector for resetting margins impacts performance and goes against the checklist recommendations. Additionally, the CSS does not utilize a variable for the blue color as required, and there are issues with the use of tag names for styling, which could affect maintainability. The HTML also contains typographical errors in the navigation links and lacks a descriptive alt attribute for the logo image. These issues affect the core requirements and functionality, so please make the necessary changes. Keep up the great effort, and I'm confident you'll get it right! 🌟
✨ 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 | ||
| data-qa="hover" | ||
| class="nav-link active" | ||
| href="#copmuters" |
There was a problem hiding this comment.
There is a typographical error in the href attribute. 'copmuters' should be corrected to 'computers'.
src/index.html
Outdated
| </li> | ||
| <li> | ||
| <a | ||
| href="#tablts" |
There was a problem hiding this comment.
There is a typographical error in the href attribute. 'tablts' should be corrected to 'tablets'.
src/style.css
Outdated
| display: flex; | ||
| } | ||
|
|
||
| .active:hover { |
There was a problem hiding this comment.
The '.active:hover' selector might not be necessary if '.is-active' is already handling the active state styling. Consider reviewing the usage of '.active' and '.is-active' to ensure they are both needed.
| color: #00acdc; | ||
| } | ||
|
|
||
| .is-active { |
There was a problem hiding this comment.
The '.is-active' class is used to style active links. Ensure that this class is consistently applied in the HTML to avoid styling inconsistencies.
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: 7]
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
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: 7]
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
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: 7]
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
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: 7]
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.
Hey, there are some things to fix. Check README.md for requirements and follow code style rules https://github.com/PrettyWisee/layout_moyo-header/blob/develop/checklist.md
src/index.html
Outdated
| href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap" | ||
| rel="stylesheet" |
There was a problem hiding this comment.
You don't need all those weights imported.
src/index.html
Outdated
| <li> | ||
| <a | ||
| data-qa="hover" | ||
| class="nav-link computers-link" |
There was a problem hiding this comment.
computers-link This class is redundant.
src/style.css
Outdated
| .computers-link:hover { | ||
| color: #00acdc; | ||
| } |
src/style.css
Outdated
| position: absolute; | ||
| height: 4px; | ||
| width: 100%; | ||
| background-color: #00acdc; |
There was a problem hiding this comment.
You are missing this requirement
CSS Variable is used for a blue color
src/style.css
Outdated
| li { | ||
| list-style: none; | ||
| margin-left: 20px; | ||
| } | ||
|
|
||
| li:first-child { | ||
| margin-left: 0; | ||
| } | ||
|
|
||
| ul { | ||
| padding: 0; | ||
| display: flex; | ||
| } | ||
|
|
||
| a { | ||
| text-decoration: none; | ||
| color: black; | ||
| } |
There was a problem hiding this comment.
Use 2 spaces for indentation in your file (not a tab character)
