- [DEMO LINK](https://katerinaasss.github.io/layout_catalog/) - [TEST REPORT LINK](https://katerinaasss.github.io/layout_catalog/report/html_report/) #4997
Conversation
There was a problem hiding this comment.
Hello! 😊 You've done a great job with the implementation, and most of your code meets the task requirements. However, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Data Attributes: The
data-qa="card"attribute should only be added to the first card, not all cards. Please ensure this is corrected to meet the task requirements. -
Navigation Link Color: The navigation links should have the color
#060b35, but currently, they are set toblackin theheader.scss. Please update this to match the specified color. -
SCSS Variables: The
header.scssfile uses hardcoded colors and sizes. These should be replaced with variables to maintain consistency and make future updates easier. -
Hover Effects: The hover effect for the card title should change the text color to
#34568b. This specific hover effect is missing in your SCSS file. Please add it to ensure the hover behavior is as expected. -
HTML and SCSS Consistency: The
.card__titleselector is used in your SCSS, but there is no corresponding element in the HTML. Please ensure that the intended element is targeted, or update the HTML to include an element with this class.
Once these issues are addressed, your code will be in great shape! Keep up the good work, and don't hesitate to reach out if you need any help. 🚀
✨ 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
| <span class="card__stars-reviews">Reviews: 5</span> | ||
| </div> | ||
|
|
||
| <p class="card__price"> | ||
| <span class="card__price-content">Price:</span> | ||
| $2,199 | ||
| </p> | ||
|
|
||
| <a | ||
| href="#" | ||
| class="card__buy" | ||
| data-qa="card-hover" | ||
| > | ||
| BUY | ||
| </a> | ||
| </div> | ||
|
|
||
| <div | ||
| class="card" | ||
| data-qa="card" | ||
| > | ||
| <img | ||
| class="card__image" | ||
| src="/src/images/imac.jpeg" | ||
| alt="imac" | ||
| /> | ||
| <h1 class="card__content"> | ||
| APPLE A1419 iMac 27" Retina | ||
| <br /> | ||
| 5K Monoblock (MNED2UA/A) | ||
| </h1> | ||
|
|
||
| <h2 class="card__code">Product code: 195434</h2> | ||
|
|
||
| <div class="card__stars"> | ||
| <div class="stars stars--4"> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| </div> | ||
| <span class="card__stars-reviews">Reviews: 5</span> | ||
| </div> | ||
|
|
||
| <p class="card__price"> | ||
| <span class="card__price-content">Price:</span> | ||
| $2,199 | ||
| </p> | ||
|
|
||
| <a | ||
| href="#" | ||
| class="card__buy" | ||
| data-qa="card-hover" | ||
| > | ||
| BUY | ||
| </a> | ||
| </div> | ||
|
|
||
| <div | ||
| class="card" | ||
| data-qa="card" | ||
| > | ||
| <img | ||
| class="card__image" | ||
| src="/src/images/imac.jpeg" | ||
| alt="imac" | ||
| /> | ||
| <h1 class="card__content"> | ||
| APPLE A1419 iMac 27" Retina | ||
| <br /> | ||
| 5K Monoblock (MNED2UA/A) | ||
| </h1> | ||
|
|
||
| <h2 class="card__code">Product code: 195434</h2> | ||
|
|
||
| <div class="card__stars"> | ||
| <div class="stars stars--4"> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| </div> | ||
| <span class="card__stars-reviews">Reviews: 5</span> | ||
| </div> | ||
|
|
||
| <p class="card__price"> | ||
| <span class="card__price-content">Price:</span> | ||
| $2,199 | ||
| </p> | ||
|
|
||
| <a | ||
| href="#" | ||
| class="card__buy" | ||
| data-qa="card-hover" | ||
| > | ||
| BUY | ||
| </a> | ||
| </div> |
There was a problem hiding this comment.
The product details, including the image, name, product code, reviews, and price, are repeated across multiple cards. If this is intentional for testing purposes, it is fine. However, if the intention is to display different products, consider using dynamic content or updating the details for each card.
| & .card__title { | ||
| color: #34568b; | ||
| } |
There was a problem hiding this comment.
The selector .card__title is used here, but there is no element with this class in the HTML file. Ensure that the intended element is targeted, or update the HTML to include an element with this class if necessary.
There was a problem hiding this comment.
Great job on addressing most of the issues from the previous review! 🎉 Your code is well-structured and meets the task requirements. However, there are a few critical points to keep in mind for future improvements:
-
SCSS Variables: Ensure all colors, such as
#00acdc,#060b35,#fff, and#f3f3f3, are defined as SCSS variables. This will help maintain consistency and ease future updates. -
Data Attributes: The
data-qa="card"attribute should only be applied to the first card. Make sure this is corrected to align with the task requirements.
Your efforts are commendable, and these adjustments will enhance the quality and maintainability of your code. 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! ✨
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
The data-qa="card" attribute should only be added to the first card, not all cards. Please ensure this is corrected to meet the task requirements.
| .card { | ||
| box-sizing: border-box; | ||
| font-family: $card-font-family; | ||
| border: 1px solid #f3f3f3; |
There was a problem hiding this comment.
The color #f3f3f3 should be replaced with an SCSS variable to maintain consistency and make future updates easier.
| width: 200px; | ||
| height: 406px; | ||
| padding: 32px 16px 16px; | ||
| background-color: #fff; |
There was a problem hiding this comment.
The color #fff should be replaced with an SCSS variable to maintain consistency and make future updates easier.
| height: 406px; | ||
| padding: 32px 16px 16px; | ||
| background-color: #fff; | ||
| color: #060b35; |
There was a problem hiding this comment.
The color #060b35 should be replaced with an SCSS variable to maintain consistency and make future updates easier.
| align-items: center; | ||
| text-transform: uppercase; | ||
| text-decoration: none; | ||
| background-color: #00acdc; |
There was a problem hiding this comment.
The color #00acdc should be replaced with an SCSS variable to maintain consistency and make future updates easier.
| &__link { | ||
| line-height: 15px; | ||
| text-align: center; | ||
| color: #060b35; |
There was a problem hiding this comment.
The color #060b35 is correctly used here, but ensure it is defined as an SCSS variable to maintain consistency and make future updates easier.
| display: flex; | ||
|
|
||
| &:hover { | ||
| color: #00acdc; |
There was a problem hiding this comment.
The color #00acdc should be replaced with an SCSS variable to maintain consistency and make future updates easier.
|
|
||
| .is-active { | ||
| position: relative; | ||
| color: #00acdc; |
There was a problem hiding this comment.
The color #00acdc should be replaced with an SCSS variable to maintain consistency and make future updates easier.
| &::after { | ||
| height: 4px; | ||
| border-radius: 8px; | ||
| background-color: #00acdc; |
There was a problem hiding this comment.
The color #00acdc should be replaced with an SCSS variable to maintain consistency and make future updates easier.
| @@ -0,0 +1,4 @@ | |||
| $card-font-family: 'Roboto', serif; | |||
| $card-color-1: #060b35; | |||
There was a problem hiding this comment.
Ensure that all colors used in your SCSS files are defined as variables here. For example, colors like #f3f3f3, #fff, and #00acdc should also be included as variables for consistency.
No description provided.