-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 #4995
base: master
Are you sure you want to change the base?
add task solution #4995
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.
Hello! 😊 You've done a great job with your code, and it's clear that you've put a lot of effort into meeting the task requirements. However, there are a few critical issues that need to be addressed before we can approve the pull request:
-
CSS Syntax Issue: The use of the
scale
property is incorrect. Please update it totransform: scale(1.2);
to ensure valid CSS syntax. -
Dynamic Content Handling: The product card section is repeated multiple times. If the content is meant to be dynamic, consider using a JavaScript framework or templating engine to generate these cards dynamically. This will help avoid repetition and improve maintainability.
These changes are important to ensure the functionality and maintainability of your code. Once you've addressed these points, feel free to resubmit your pull request. Keep up the great work, and I'm looking forward to seeing your improvements! 🚀
✨ 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> | ||
|
||
<button class="product-card__button">BUY</button> | ||
</div> | ||
|
||
<div class="product-card"> | ||
<img | ||
class="product-card__image" | ||
src="images/imac.jpeg" | ||
alt="Product logo" | ||
/> | ||
|
||
<h2 class="product-card__name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
|
||
<p class="product-card__code">Product code: 195434</p> | ||
|
||
<div class="product-card__info"> | ||
<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="product-card__review">Reviews: 5</p> | ||
</div> | ||
|
||
<div class="product-card__price"> | ||
<p class="product-card__price-title">Price:</p> | ||
<p class="product-card__price-value">$2,199</p> | ||
</div> | ||
|
||
<button class="product-card__button">BUY</button> | ||
</div> | ||
|
||
<div class="product-card"> | ||
<img | ||
class="product-card__image" | ||
src="images/imac.jpeg" | ||
alt="Product logo" | ||
/> | ||
|
||
<h2 class="product-card__name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
|
||
<p class="product-card__code">Product code: 195434</p> | ||
|
||
<div class="product-card__info"> | ||
<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="product-card__review">Reviews: 5</p> | ||
</div> | ||
|
||
<div class="product-card__price"> | ||
<p class="product-card__price-title">Price:</p> | ||
<p class="product-card__price-value">$2,199</p> | ||
</div> | ||
|
||
<button class="product-card__button">BUY</button> | ||
</div> | ||
|
||
<div class="product-card"> | ||
<img | ||
class="product-card__image" | ||
src="images/imac.jpeg" | ||
alt="Product logo" | ||
/> | ||
|
||
<h2 class="product-card__name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
|
||
<p class="product-card__code">Product code: 195434</p> | ||
|
||
<div class="product-card__info"> | ||
<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="product-card__review">Reviews: 5</p> | ||
</div> | ||
|
||
<div class="product-card__price"> | ||
<p class="product-card__price-title">Price:</p> | ||
<p class="product-card__price-value">$2,199</p> | ||
</div> | ||
|
||
<button class="product-card__button">BUY</button> | ||
</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 product card section is repeated multiple times with the same content. If this is intentional for static content, it's fine. However, if the data is dynamic, consider using a JavaScript framework or templating engine to generate these cards dynamically to avoid repetition and improve maintainability.
src/styles/index.scss
Outdated
.product-card:hover { | ||
scale: (1.2); |
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
property is not valid CSS. To scale an element, use transform: scale(1.2);
instead.
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 previous issues! 🎉 However, there are still a few points that need attention to enhance your code's quality and maintainability:
-
Dynamic Content Handling: The product card section is repeated multiple times in the HTML file. If the content is meant to be dynamic, consider using a JavaScript framework or templating engine to generate these cards dynamically. This will help avoid repetition and improve maintainability.
-
SCSS Variables: While your SCSS file uses variables for some colors, ensure that all repeated sizes and special colors are put into variables as per the checklist requirement.
-
CSS Syntax: The use of the
scale
property is incorrect. Please update it totransform: scale(1.2);
to ensure valid CSS syntax. -
BEM and SCSS Consistency: Ensure that all components follow BEM and use SCSS consistently across all components.
Your effort is commendable, and the decision to approve is made 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 good 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> | ||
|
||
<button class="product-card__button">BUY</button> | ||
</div> | ||
|
||
<div class="product-card"> | ||
<img | ||
class="product-card__image" | ||
src="images/imac.jpeg" | ||
alt="Product logo" | ||
/> | ||
|
||
<h2 class="product-card__name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
|
||
<p class="product-card__code">Product code: 195434</p> | ||
|
||
<div class="product-card__info"> | ||
<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="product-card__review">Reviews: 5</p> | ||
</div> | ||
|
||
<div class="product-card__price"> | ||
<p class="product-card__price-title">Price:</p> | ||
<p class="product-card__price-value">$2,199</p> | ||
</div> | ||
|
||
<button class="product-card__button">BUY</button> | ||
</div> | ||
|
||
<div class="product-card"> | ||
<img | ||
class="product-card__image" | ||
src="images/imac.jpeg" | ||
alt="Product logo" | ||
/> | ||
|
||
<h2 class="product-card__name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
|
||
<p class="product-card__code">Product code: 195434</p> | ||
|
||
<div class="product-card__info"> | ||
<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="product-card__review">Reviews: 5</p> | ||
</div> | ||
|
||
<div class="product-card__price"> | ||
<p class="product-card__price-title">Price:</p> | ||
<p class="product-card__price-value">$2,199</p> | ||
</div> | ||
|
||
<button class="product-card__button">BUY</button> | ||
</div> | ||
|
||
<div class="product-card"> | ||
<img | ||
class="product-card__image" | ||
src="images/imac.jpeg" | ||
alt="Product logo" | ||
/> | ||
|
||
<h2 class="product-card__name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
|
||
<p class="product-card__code">Product code: 195434</p> | ||
|
||
<div class="product-card__info"> | ||
<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="product-card__review">Reviews: 5</p> | ||
</div> | ||
|
||
<div class="product-card__price"> | ||
<p class="product-card__price-title">Price:</p> | ||
<p class="product-card__price-value">$2,199</p> | ||
</div> | ||
|
||
<button class="product-card__button">BUY</button> | ||
</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 product card section is repeated multiple times. If the content is meant to be dynamic, consider using a JavaScript framework or templating engine to generate these cards dynamically. This will help avoid repetition and improve maintainability.
.product-card:hover { | ||
transform: scale(1.2); |
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 use of the scale
property is incorrect. Please update it to transform: scale(1.2);
to ensure valid CSS syntax.
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.
Good job, but please try to fix comments and pass all tests. Feel free to ask for help in fe_chat.
align-items: center; | ||
justify-content: space-between; | ||
padding: 0 50px; | ||
text-align: center; |
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.
text-align: center; |
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.
Redundant style, please remove it
display: block; | ||
width: 40px; | ||
height: 40px; | ||
padding: 10px 0; |
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.
padding: 10px 0; |
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.
You don't need to use padding as you have align-items on header
font-size: 12px; | ||
font-weight: 500; | ||
text-align: center; | ||
height: 60px; |
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.
height: 60px; |
Redundant style, please remove it
text-transform: uppercase; | ||
font-weight: 500; | ||
margin-left: 20px; | ||
display: inline-block; | ||
align-items: center; | ||
position: relative; |
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.
text-transform: uppercase; | |
font-weight: 500; | |
margin-left: 20px; | |
display: inline-block; | |
align-items: center; | |
position: relative; | |
text-transform: uppercase; | |
margin-left: 20px; |
.main { | ||
display: grid; | ||
padding: 50px 40px; | ||
grid-template-columns: repeat(1, auto); |
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.
grid-template-columns: repeat(1, auto); | |
grid-template-columns: repeat(1, 200px); |
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 same for other media queries
padding-top: 32px; | ||
padding-left: 19px; | ||
padding-right: 19px; |
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.
use paddings on card instead of using it on each element inside
margin-bottom: 40px; | ||
} | ||
&__name { | ||
width: 166px; |
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.
This element should be 100% width by default so you can remove width at all
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.
Same for other p tags
:hover
are smooth