-
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 #4847
base: master
Are you sure you want to change the base?
add task solution #4847
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 code! There are a few minor issues to address and some areas for improvement, but none of them are critical or affect the functionality of the code. Keep up the good work! Remember, coding is a journey of continuous learning and improvement. 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<link | ||
rel="stylesheet" | ||
href="./styles/index.scss" | ||
/> |
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 have two identical links to the index.scss
file. Please, remove one of them.
|
||
border-radius: 5px; | ||
border: 1px solid rgba(243, 243, 243, 1); | ||
box-sizing: border-box; |
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 have specified box-sizing: border-box;
for this class. While this is not inherently wrong, it is a good practice to apply this property to all elements at once in your general or reset stylesheet. This helps to ensure consistent sizing of all elements in your project.
|
||
& img { | ||
width: 100%; | ||
height: 134px; |
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's not recommended to set the height of the image. This can lead to distortion if the aspect ratio of the original image is not the same as the one you've set. It's better to set only the width and let the height be set automatically to maintain the aspect ratio of the image.
background-repeat: no-repeat; | ||
width: 16px; | ||
height: 16px; | ||
margin-right: 4px; |
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 margin-right: 4px;
style will be applied to the last star as well, which could potentially cause alignment issues. A common solution is to use margin-left: 4px;
and then offset the first child with &:first-child { margin-left: 0; }
.
} | ||
|
||
&__star:nth-child(-n + 4) { | ||
background-image: url('/src/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.
This selector &__star:nth-child(-n + 4)
will always select the first 4 stars, regardless of the rating. The number of active stars should be determined dynamically, based on the rating data. You might need to adjust your HTML structure and/or CSS to achieve this.
&__button { | ||
height: 40px; | ||
width: 100%; | ||
border: none; |
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.
Setting the width to 100% could cause layout issues if the parent container's padding, border, or margin is not accounted for. It might be better to use width: auto;
and control the button's width with padding.
background-color: #fff; | ||
border: 1px solid $color-button; | ||
color: $color-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.
On hover, the button text color changes to $color-button
, which is the same color as the border. This could potentially reduce the readability of the text. Consider choosing a color that contrasts with the border color.
margin-right: 4px; | ||
} | ||
|
||
&__star: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.
This code will always make the first 4 stars active, no matter how many stars you want to be active. You should add a modifier to the stars block, like 'stars--4', and then use it in combination with the nth-child selector to select the appropriate number of active stars. For example, '.stars--4 &__star:nth-child(-n + 4)'.
DEMO LINK
TEST REPORT LINK
[ x ] there 2 BEM blocks
card
andstars
each in it own file[ x ] SCSS Nesting is used for
elements
,modifiers
andpseudo-classes
[ x ] SCSS Variables are used for main values and placed in a separate file
[ x ] all
stars--N
modifiers work as expected (Highlight firstN
stars)[ x ] Code follows all the Code Style Rules ❗️