-
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 solution #4851
base: master
Are you sure you want to change the base?
add solution #4851
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 project! 😊 While your implementation is mostly correct and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Grid Layout Issue: The
grid-template-columns
property is currently set to$card-width
, which results in a single column grid. To meet the task requirements for a responsive grid layout, consider using therepeat()
function or specifying multiple column widths directly to allow for multiple columns. -
Hover Effects: The hover effect for scaling is set to 120%, which is quite large and may disrupt the layout. Please adjust this to a smaller scale factor to maintain visual appeal and ensure it doesn't affect the surrounding layout.
-
Code Redundancy: The product cards are repeated with the same content. To improve maintainability, consider using a templating engine or JavaScript to dynamically generate these cards. This will make your code easier to update and manage.
Please address these issues to ensure the functionality and maintainability of your code. Once these changes are made, we can proceed with the approval. Keep up the great work, and don't hesitate to reach out if you need any assistance! 🚀
✨ 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.
<div class="card__reviews"> | ||
<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> | ||
<p class="card__reviews-count">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-text">Price:</p> | ||
<p class="card__dollar">$2,199</p> | ||
</div> | ||
<button class="card__button">Buy</button> | ||
</article> | ||
<article class="card"> | ||
<img | ||
class="card__img" | ||
src="./images/imac.jpeg" | ||
alt="APPLE A1419 iMac 27 Retina" | ||
/> | ||
<div class="card__description"> | ||
<h2 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
<p class="card__code">Product code: 195434</p> | ||
</div> | ||
<div class="card__reviews"> | ||
<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> | ||
<p class="card__reviews-count">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-text">Price:</p> | ||
<p class="card__dollar">$2,199</p> | ||
</div> | ||
<button class="card__button">Buy</button> | ||
</article> | ||
<article class="card"> | ||
<img | ||
class="card__img" | ||
src="./images/imac.jpeg" | ||
alt="APPLE A1419 iMac 27 Retina" | ||
/> | ||
<div class="card__description"> | ||
<h2 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
<p class="card__code">Product code: 195434</p> | ||
</div> | ||
<div class="card__reviews"> | ||
<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> | ||
<p class="card__reviews-count">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-text">Price:</p> | ||
<p class="card__dollar">$2,199</p> | ||
</div> | ||
<button class="card__button">Buy</button> | ||
</article> | ||
<article class="card"> | ||
<img | ||
class="card__img" | ||
src="./images/imac.jpeg" | ||
alt="APPLE A1419 iMac 27 Retina" | ||
/> | ||
<div class="card__description"> | ||
<h2 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
<p class="card__code">Product code: 195434</p> | ||
</div> | ||
<div class="card__reviews"> | ||
<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> | ||
<p class="card__reviews-count">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-text">Price:</p> | ||
<p class="card__dollar">$2,199</p> | ||
</div> | ||
<button class="card__button">Buy</button> | ||
</article> |
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 product cards are repeated multiple times with the same content. This repetition can be optimized by using a templating engine or JavaScript to dynamically generate these cards. This will make the code more maintainable and easier to update.
src/styles/blocks/card.scss
Outdated
background 300ms; | ||
|
||
&:hover { | ||
transform: scale(120%); |
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 scale transformation on hover is set to 120%, which might be too large and could affect the layout. Consider using a smaller scale factor to ensure the card remains visually appealing and doesn't disrupt the surrounding layout.
src/styles/blocks/card.scss
Outdated
|
||
&:hover .card__button { | ||
background: $white-color; | ||
border-radius: 5px; |
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 is being redefined on hover, which is unnecessary since it's already set on the button. You can remove this line to simplify the hover effect.
src/styles/blocks/container.scss
Outdated
.container { | ||
display: grid; | ||
gap: 46px 48px; | ||
grid-template-columns: $card-width; |
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 grid-template-columns property is set to $card-width, which will create a single column grid. If the intention is to have multiple columns, consider using the repeat() function or specifying multiple column widths directly.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
Great progress on your project! 🎉 You've effectively addressed many of the previous issues, particularly with the grid layout and hover effects. The grid now uses the repeat()
function for a responsive design, and the hover effect is more appropriate at 110%. However, there are still some areas for improvement:
-
Code Redundancy: The product cards in your HTML file are still repeated with the same content. To enhance maintainability, consider using a templating engine or JavaScript to dynamically generate these cards. This will make your code easier to update and manage.
-
Hover Effects: While the hover effect scale is set to 110%, it might still be too large and could disrupt the layout. Consider reducing the scale factor slightly to maintain visual appeal and ensure it doesn't affect the surrounding layout.
Despite these issues, the decision is to APPROVE your pull request to help you move forward with your learning. Please review the comments to improve your code. Addressing these issues will be crucial for future projects to avoid similar problems. 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! ✨
</div> | ||
<div class="card__reviews"> | ||
<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> | ||
<p class="card__reviews-count">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-text">Price:</p> | ||
<p class="card__dollar">$2,199</p> | ||
</div> | ||
<button class="card__button">Buy</button> | ||
</article> | ||
<article class="card"> | ||
<img | ||
class="card__img" | ||
src="./images/imac.jpeg" | ||
alt="APPLE A1419 iMac 27 Retina" | ||
/> | ||
<div class="card__description"> | ||
<h2 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
<p class="card__code">Product code: 195434</p> | ||
</div> | ||
<div class="card__reviews"> | ||
<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> | ||
<p class="card__reviews-count">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-text">Price:</p> | ||
<p class="card__dollar">$2,199</p> | ||
</div> | ||
<button class="card__button">Buy</button> | ||
</article> | ||
<article class="card"> | ||
<img | ||
class="card__img" | ||
src="./images/imac.jpeg" | ||
alt="APPLE A1419 iMac 27 Retina" | ||
/> | ||
<div class="card__description"> | ||
<h2 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
<p class="card__code">Product code: 195434</p> | ||
</div> | ||
<div class="card__reviews"> | ||
<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> | ||
<p class="card__reviews-count">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-text">Price:</p> | ||
<p class="card__dollar">$2,199</p> | ||
</div> | ||
<button class="card__button">Buy</button> | ||
</article> | ||
<article class="card"> | ||
<img | ||
class="card__img" | ||
src="./images/imac.jpeg" | ||
alt="APPLE A1419 iMac 27 Retina" | ||
/> | ||
<div class="card__description"> | ||
<h2 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
<p class="card__code">Product code: 195434</p> | ||
</div> | ||
<div class="card__reviews"> | ||
<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> | ||
<p class="card__reviews-count">Reviews: 5</p> | ||
</div> | ||
<div class="card__price"> | ||
<p class="card__price-text">Price:</p> | ||
<p class="card__dollar">$2,199</p> | ||
</div> | ||
<button class="card__button">Buy</button> |
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 product cards are repeated with the same content. To improve maintainability, consider using a templating engine or JavaScript to dynamically generate these cards. This will make your code easier to update and manage.
&:hover { | ||
transform: scale(110%); |
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 hover effect for scaling is set to 110%, which may still be too large and could disrupt the layout. Please adjust this to a smaller scale factor to maintain visual appeal and ensure it doesn't affect the surrounding layout.
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 ❗️