-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Create product card #5125
base: master
Are you sure you want to change the base?
Create product card #5125
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.
Great job on your work! 🎉 The code is well-structured and meets most of the task requirements. Just a small note: make sure to correct the typo in the variable name from $main-font-weigh
to $main-font-weight
for consistency. Keep up the great work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
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, there are a few comments to fix. Please feel free to ask for help in fe_chat.
data-qa="card" | ||
> | ||
<div class="card__content"> | ||
<div class="card__item-image"></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.
use image tag to show imac picture.
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 so much for review and suggestions, will fix it :)
class="card" | ||
data-qa="card" | ||
> | ||
<div class="card__content"> |
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 tag is redundant here, please remove it.
> | ||
<div class="card__content"> | ||
<div class="card__item-image"></div> | ||
<div class="card__item"> |
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.
<div class="card__item"> |
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 tag is redundant here, please remove it.
class="card" | ||
data-qa="card" | ||
> | ||
<div class="card__content"> |
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.
<div class="card__content"> |
<div class="card__item-name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</div> | ||
<div class="card__item-code">Product code: 195434</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.
use semantic tags for text. Please make it for everywhere.
<div class="card__item-name"> | |
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | |
</div> | |
<div class="card__item-code">Product code: 195434</div> | |
<h3 class="card__item-name"> | |
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | |
</h3> | |
<pclass="card__item-code">Product code: 195434</p> |
<div | ||
class="buy" | ||
data-qa="hover" | ||
> | ||
<a class="buy__button">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.
<div | |
class="buy" | |
data-qa="hover" | |
> | |
<a class="buy__button">Buy</a> | |
</div> | |
<a class="buy__button">Buy</a> |
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 a link without div cover tag
.card { | ||
width: 200px; | ||
height: 400px; | ||
margin: 40px; |
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 margin is redundant here, please remove it
height: 400px; | ||
margin: 40px; | ||
|
||
padding: 32px 19px 16px; |
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: 32px 19px 16px; | |
padding: 32px 16px 16px; |
&-stars { | ||
background-image: url(../images/star.svg); | ||
background-repeat: no-repeat; | ||
width: 16px; | ||
height: 16px; | ||
padding: 2px; |
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.
&-stars { | |
background-image: url(../images/star.svg); | |
background-repeat: no-repeat; | |
width: 16px; | |
height: 16px; | |
padding: 2px; | |
&-stars { | |
background-image: url(../images/star.svg); | |
background-repeat: no-repeat; | |
background-position: center; | |
width: 16px; | |
height: 16px; | |
margin-right: 4px; |
DEMO LINK
TEST REPORT LINK
there 2 BEM blocks
card
andstars
each in their own fileSCSS Nesting is used for
elements
,modifiers
andpseudo-classes
SCSS Variables are used for main values and placed in a separate file
all
stars--N
modifiers work as expected (Highlight firstN
stars)Code follows all the Code Style Rules ❗️