-
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 solution #4905
base: master
Are you sure you want to change the base?
add solution #4905
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 the SCSS files! 🎉 The issues noted are minor style inconsistencies that don't affect the functionality. Consider adjusting the spacing and nesting for improved code style in future updates. Keep up the good work!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
@include blocks-width; | ||
|
||
&_label { |
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.
It seems like the selector for &_label
is nested under &__price
. Ensure that this is intentional and aligns with your design requirements. If &_label
is meant to be a standalone element, consider adjusting its placement.
color: $second-text-color; | ||
} | ||
|
||
&_value { |
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.
Similar to the previous comment, verify that &_value
is correctly nested under &__price
. If it should be a separate element, you might want to adjust its nesting.
margin-right: 4px; | ||
} | ||
|
||
&--1 :nth-child(-n + 1) { |
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.
There is an extra space between '&--1' and ':nth-child(-n + 1)'. According to the code style rules, there should be no space between the element and the pseudo-class.
background-image: url(../images/star-active.svg) !important; | ||
} | ||
|
||
&--2 :nth-child(-n + 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.
There is an extra space between '&--2' and ':nth-child(-n + 2)'. According to the code style rules, there should be no space between the element and the pseudo-class.
background-image: url(../images/star-active.svg); | ||
} | ||
|
||
&--3 :nth-child(-n + 3) { |
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.
There is an extra space between '&--3' and ':nth-child(-n + 3)'. According to the code style rules, there should be no space between the element and the pseudo-class.
background-image: url(../images/star-active.svg); | ||
} | ||
|
||
&--4 :nth-child(-n + 4) { |
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.
There is an extra space between '&--4' and ':nth-child(-n + 4)'. According to the code style rules, there should be no space between the element and the pseudo-class.
background-image: url(../images/star-active.svg); | ||
} | ||
|
||
&--5 :nth-child(-n + 5) { |
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.
There is an extra space between '&--5' and ':nth-child(-n + 5)'. According to the code style rules, there should be no space between the element and the pseudo-class.
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.
Overall - good, but let's make some fixes 🥹
<img | ||
class="product-card__image" | ||
src="images/imac.jpeg" | ||
alt="iMac Image" | ||
/> | ||
<a | ||
class="product-card__name" | ||
href="#" | ||
> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</a> | ||
<p class="product-card__code">Product code: 195434</p> |
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.
don't forget about indents. It will be more readable
<img | |
class="product-card__image" | |
src="images/imac.jpeg" | |
alt="iMac Image" | |
/> | |
<a | |
class="product-card__name" | |
href="#" | |
> | |
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | |
</a> | |
<p class="product-card__code">Product code: 195434</p> | |
<img | |
class="product-card__image" | |
src="images/imac.jpeg" | |
alt="iMac Image" | |
/> | |
<a | |
class="product-card__name" | |
href="#" | |
> | |
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | |
</a> | |
<p class="product-card__code">Product code: 195434</p> |
body { | ||
margin: 0; | ||
padding: 0; | ||
display: flex; | ||
justify-content: center; | ||
font-family: Roboto, sans-serif; | ||
} |
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.
don't use positioning on body tag. Use a separate class - .body
for it
body { | |
margin: 0; | |
padding: 0; | |
display: flex; | |
justify-content: center; | |
font-family: Roboto, sans-serif; | |
} | |
body { | |
margin: 0; | |
padding: 0; | |
font-family: Roboto, sans-serif; | |
} |
@@ -0,0 +1,109 @@ | |||
body { |
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.
u can move it to reset.scss
file in which u will be reset values and only this
} | ||
|
||
.product-card { | ||
background-color: #fff; |
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.
create variables for all colors
font-weight: 400; | ||
line-height: 14px; | ||
color: $second-text-color; | ||
margin: 4px 0 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.
margin top?
background-image: url(../images/star.svg); | ||
background-repeat: no-repeat; | ||
background-position: 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.
u can use just background
. Check doc - background
background-image: url(../images/star.svg); | |
background-repeat: no-repeat; | |
background-position: center; | |
background: ....... |
&--1 :nth-child(-n + 1) { | ||
background-image: url(../images/star-active.svg) !important; | ||
} | ||
|
||
&--2 :nth-child(-n + 2) { | ||
background-image: url(../images/star-active.svg); | ||
} | ||
|
||
&--3 :nth-child(-n + 3) { | ||
background-image: url(../images/star-active.svg); | ||
} | ||
|
||
&--4 :nth-child(-n + 4) { | ||
background-image: url(../images/star-active.svg); | ||
} | ||
|
||
&--5 :nth-child(-n + 5) { | ||
background-image: url(../images/star-active.svg); | ||
} |
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.
- Never use
!important
!!!
this may lead to conflicts in the future - U can modify ur syntax
&--1 :nth-child(-n + 1) { | |
background-image: url(../images/star-active.svg) !important; | |
} | |
&--2 :nth-child(-n + 2) { | |
background-image: url(../images/star-active.svg); | |
} | |
&--3 :nth-child(-n + 3) { | |
background-image: url(../images/star-active.svg); | |
} | |
&--4 :nth-child(-n + 4) { | |
background-image: url(../images/star-active.svg); | |
} | |
&--5 :nth-child(-n + 5) { | |
background-image: url(../images/star-active.svg); | |
} | |
&--1 :nth-child(1), | |
&--2 :nth-child(-n + 2), | |
&--3 :nth-child(-n + 3), | |
&--4 :nth-child(-n + 4), | |
&--5 :nth-child(-n + 5) { | |
background-image: url(../images/star-active.svg); | |
} |
color: $main-text-color; | ||
margin-top: 40px; | ||
|
||
@include blocks-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.
this is not correct. Don't hardcode ur width. This must be a dynamic value
For all the cases!
@include blocks-width; | |
width: 100% |
$card-border-width: 1px; | ||
$card-padding: 16px; | ||
$card-width: 200px; | ||
$blocks-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.
u don't need this variables. It doesn't make sense
$card-border-width: 1px; | |
$card-padding: 16px; | |
$card-width: 200px; | |
$blocks-width: 166px; |
DEMO LINK
[TEST REPORT LINK](https://save-v.github.io/layout_product-cards/report/
there 2 BEM blocks card and stars each in it own file
SCSS Nesting is used for elements, modifiers and pseudo-classes
SCSS Variables are used for main values and placed in a separate file
all stars--N modifiers work as expected (Highlight first N stars)
Code follows all the Code Style Rules ❗️