-
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
production-card #4906
base: master
Are you sure you want to change the base?
production-card #4906
Conversation
gabai62
commented
Oct 20, 2024
- DEMO LINK
- TEST REPORT LINK
- 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 ❗️
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 are a few issues that need to be addressed before approval. Please fix the spacing issues according to the code style rules and correct the 'align-items' value to ensure proper functionality. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/styles/blocks/card.scss
Outdated
&__count-review { | ||
display: flex; | ||
justify-content: flex-end; | ||
align-items: end; |
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 value 'end' for 'align-items' is not valid. You might want to use 'flex-end' instead. According to the CSS Flexbox specification, 'align-items' accepts values like 'flex-start', 'flex-end', 'center', etc.
src/styles/blocks/star.scss
Outdated
background-repeat: no-repeat; | ||
} | ||
|
||
&--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.
src/styles/blocks/star.scss
Outdated
background-image: url(../images/star-active.svg); | ||
} | ||
|
||
& :last-of-type { |
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 '&' and ':last-of-type'. 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.
Great job on structuring the SCSS file correctly and following the BEM methodology! 🎉 The use of nested selectors and color variables is spot on. While there is a minor inconsistency in the margin values, it's a good opportunity to review and ensure consistency in your styling. Keep up the good work and continue to refine your skills!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
margin-top: 16px; | ||
margin-bottom: 15px; |
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 inconsistency in the margin values. The margin-top
is set to 16px, while the margin-bottom
is set to 15px. If this is intentional, it's fine, but if not, consider making them consistent.