-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add task solution #4839
base: master
Are you sure you want to change the base?
add task solution #4839
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on this pull request! 😊 While your code meets most of the task requirements, there are a few critical issues that need to be addressed before we can approve it:
-
SCSS Nesting Issue: The
.card__title
is incorrectly nested within its own:hover
pseudo-class. This should be corrected by directly applying the color change within the:hover
block without nesting. -
Duplicate CSS Properties: The
border-radius
property is defined twice for the.card
class. Please remove the duplicate declaration to ensure consistency and avoid confusion. -
HTML Redundancy: The card elements are repeated multiple times with the same content. Consider using a templating engine or JavaScript to dynamically generate these cards. This will make your code more maintainable and reduce redundancy.
Please address these issues to enhance the functionality and maintainability of your code. Once these changes are made, we can proceed with the approval. 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.
class="card__image" | ||
alt="Picture of iMac 27 inch Retina" | ||
/> | ||
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__product-code">Product code: 195434</p> | ||
<div class="card__review"> | ||
<div class="card__stars stars stars--4"> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
</div> | ||
<p class="card__review-text">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-title">Price:</p> | ||
<p class="card__price-numbers">$2,199</p> | ||
</div> | ||
<a | ||
href="#" | ||
class="card__link" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
|
||
<!-- Card 7 --> | ||
<div class="catalog__card card"> | ||
<img | ||
src="./images/imac.jpeg" | ||
class="card__image" | ||
alt="Picture of iMac 27 inch Retina" | ||
/> | ||
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__product-code">Product code: 195434</p> | ||
<div class="card__review"> | ||
<div class="card__stars stars stars--4"> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
</div> | ||
<p class="card__review-text">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-title">Price:</p> | ||
<p class="card__price-numbers">$2,199</p> | ||
</div> | ||
<a | ||
href="#" | ||
class="card__link" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
|
||
<!-- Card 8 --> | ||
<div class="catalog__card card"> | ||
<img | ||
src="./images/imac.jpeg" | ||
class="card__image" | ||
alt="Picture of iMac 27 inch Retina" | ||
/> | ||
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__product-code">Product code: 195434</p> | ||
<div class="card__review"> | ||
<div class="card__stars stars stars--4"> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
</div> | ||
<p class="card__review-text">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-title">Price:</p> | ||
<p class="card__price-numbers">$2,199</p> | ||
</div> | ||
<a | ||
href="#" | ||
class="card__link" | ||
> | ||
Buy | ||
</a> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The card elements are repeated multiple times with the same content. Consider using a templating engine or JavaScript to dynamically generate these cards to avoid redundancy and make the code more maintainable.
src/styles/blocks/catalog.scss
Outdated
border-radius: $border-radius; | ||
padding: 32px 16px 16px; | ||
position: relative; | ||
border-radius: 8px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'border-radius' property is defined twice for the '.card' class. Consider removing the duplicate declaration to avoid confusion and ensure consistency.
src/styles/blocks/catalog.scss
Outdated
&:hover { | ||
transform: scale(1.2); | ||
z-index: 10; | ||
|
||
.card__title { | ||
color: #34568b; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nesting of '.card__title' inside its own ':hover' pseudo-class is incorrect. The '.card__title' should not be nested within ':hover' as it is already being targeted. Consider removing the nested '.card__title' and directly applying the color change within the ':hover' block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on addressing most of the previous issues! 🎉 Your SCSS files are well-structured and follow a clear BEM naming convention, which is excellent for maintainability and readability. However, there are a few points to consider for further improvement:
-
HTML Redundancy: The HTML file still contains repeated card elements with identical content. To enhance maintainability, consider using a templating engine or JavaScript to dynamically generate these cards.
-
SCSS Nesting Issue: Ensure that the
.card__title
hover effect is correctly applied without incorrect nesting. It should be directly applied within the:hover
block. -
Duplicate CSS Properties: The
border-radius
property was previously duplicated in the.card
class. Make sure to remove any duplicate declarations to maintain consistency.
These adjustments will help you improve your code quality 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! ✨
class="card__image" | ||
alt="Picture of iMac 27 inch Retina" | ||
/> | ||
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__product-code">Product code: 195434</p> | ||
<div class="card__review"> | ||
<div class="card__stars stars stars--4"> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
</div> | ||
<p class="card__review-text">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-title">Price:</p> | ||
<p class="card__price-numbers">$2,199</p> | ||
</div> | ||
<a | ||
href="#" | ||
class="card__link" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
|
||
<!-- Card 7 --> | ||
<div class="catalog__card card"> | ||
<img | ||
src="./images/imac.jpeg" | ||
class="card__image" | ||
alt="Picture of iMac 27 inch Retina" | ||
/> | ||
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__product-code">Product code: 195434</p> | ||
<div class="card__review"> | ||
<div class="card__stars stars stars--4"> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
</div> | ||
<p class="card__review-text">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-title">Price:</p> | ||
<p class="card__price-numbers">$2,199</p> | ||
</div> | ||
<a | ||
href="#" | ||
class="card__link" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
|
||
<!-- Card 8 --> | ||
<div class="catalog__card card"> | ||
<img | ||
src="./images/imac.jpeg" | ||
class="card__image" | ||
alt="Picture of iMac 27 inch Retina" | ||
/> | ||
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__product-code">Product code: 195434</p> | ||
<div class="card__review"> | ||
<div class="card__stars stars stars--4"> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
</div> | ||
<p class="card__review-text">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-title">Price:</p> | ||
<p class="card__price-numbers">$2,199</p> | ||
</div> | ||
<a | ||
href="#" | ||
class="card__link" | ||
> | ||
Buy | ||
</a> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The card elements are repeated multiple times with the same content. Consider using a templating engine or JavaScript to dynamically generate these cards. This will make your code more maintainable and reduce redundancy.
&:hover .card__title { | ||
color: #34568b; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .card__title
is incorrectly nested within its own :hover
pseudo-class. This should be corrected by directly applying the color change within the :hover
block without nesting.
background: $color-bg-white; | ||
width: $card-width; | ||
border: 1px solid #f3f3f3; | ||
border-radius: $border-radius; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The border-radius
property is defined twice for the .card
class. Please remove the duplicate declaration to ensure consistency and avoid confusion.
DEMO LINK
TEST REPORT LINK
All components follow BEM and use SCSS
Repeated sizes and special colors are put to variables
Grid is used for the columns
Cards are shown in 1, 2, 3, or 4 columns based on screen resolution
All changes on
:hover
are smoothCode follows all the Code Style Rules ❗️