add solution #4852
add solution #4852
Conversation
JosielLinhar
commented
Nov 13, 2024
JosielLinhar
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Thank you for your hard work on this project! There are a couple of critical issues that need to be addressed before we can approve the pull request. The data-qa attribute inconsistency needs to be resolved for better maintainability, and it's crucial to ensure that all necessary SCSS variables like $active-color are defined to prevent compilation errors. Once these are fixed, 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.
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
The data-qa attribute here is set to card-hover, which is inconsistent with the other similar buttons that use data-qa="hover". Ensure consistency in naming conventions for easier maintenance and understanding.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
The data-qa attribute here is set to hover, which is inconsistent with the first card button that uses data-qa="card-hover". Ensure consistency in naming conventions for easier maintenance and understanding.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
The data-qa attribute here is set to hover, which is inconsistent with the first card button that uses data-qa="card-hover". Ensure consistency in naming conventions for easier maintenance and understanding.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
The data-qa attribute here is set to hover, which is inconsistent with the first card button that uses data-qa="card-hover". Ensure consistency in naming conventions for easier maintenance and understanding.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
The data-qa attribute here is set to hover, which is inconsistent with the first card button that uses data-qa="card-hover". Ensure consistency in naming conventions for easier maintenance and understanding.
| &:hover &__button { | ||
| background-color: #fff; | ||
| color: $active-color; | ||
| border: 1px solid $active-color; |
There was a problem hiding this comment.
Ensure that the $active-color variable is defined in your SCSS files. If it's not defined, it will cause a compilation error. Check your variables file or define it at the top of this file.
| font-weight: 500; | ||
| text-decoration: none; | ||
| text-transform: uppercase; | ||
| color: $normal-color; |
There was a problem hiding this comment.
Ensure that the $normal-color variable is defined in your SCSS files. If it's not defined, it will cause a compilation error. Check your variables file or define it at the top of this file.
| color: $normal-color; | ||
|
|
||
| &:hover { | ||
| color: $active-color; |
There was a problem hiding this comment.
Ensure that the $active-color variable is defined in your SCSS files. If it's not defined, it will cause a compilation error. Check your variables file or define it at the top of this file.
| } | ||
|
|
||
| .is-active { | ||
| color: $active-color; |
There was a problem hiding this comment.
Ensure that the $active-color variable is defined in your SCSS files. If it's not defined, it will cause a compilation error. Check your variables file or define it at the top of this file.
| bottom: 0; | ||
| left: 0; | ||
|
|
||
| background-color: $active-color; |
There was a problem hiding this comment.
Ensure that the $active-color variable is defined in your SCSS files. If it's not defined, it will cause a compilation error. Check your variables file or define it at the top of this file.
There was a problem hiding this comment.
Great job on addressing the previous issues and ensuring that your code meets the task requirements! 🎉 Your HTML structure is well-organized, and you've successfully updated the data-qa attributes as needed. The SCSS variables $active-color and $normal-color are correctly defined, which resolves previous concerns. Your use of BEM methodology, SCSS, and responsive design principles is commendable.
However, there are a few areas to improve:
- Ensure consistent use of the
data-qaattribute across all card buttons and navigation links for better maintainability and testing. - Make sure that all SCSS variables are defined to avoid any potential compilation errors.
These improvements will enhance your code's functionality and maintainability. Keep up the great work, and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| data-qa="nav-hover" |
There was a problem hiding this comment.
Inconsistent use of data-qa attribute. Consider adding data-qa attributes to all navigation links for consistency and easier testing.
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
Inconsistent use of data-qa attribute. Consider adding data-qa attributes to all card buttons for consistency and easier testing.
| background-color: $active-color; | ||
| border-radius: 5px; | ||
| } | ||
|
|
||
| &:hover &__button { | ||
| background-color: #fff; | ||
| color: $active-color; | ||
| border: 1px solid $active-color; |
There was a problem hiding this comment.
Ensure that the $active-color variable is defined in your SCSS files. This variable is used for styling the button and hover effects, and its absence can lead to compilation errors. It is defined in src/styles/utils/variables.scss as $active-color: #00acdc;.
| font-weight: 500; | ||
| text-decoration: none; | ||
| text-transform: uppercase; | ||
| color: $normal-color; |
There was a problem hiding this comment.
Ensure that the $normal-color variable is defined in your SCSS files. It is used for styling the navigation link color and is defined as $normal-color: #060b35; in src/styles/utils/variables.scss.
