Develop #4826
Develop #4826
Conversation
There was a problem hiding this comment.
Great job on structuring your styles and following the BEM methodology! 👍 Just a few minor suggestions on code style and consistency. Remember to define common variables for maintainability and to keep following the code style guide for SCSS. Keep up the good work! 🌟
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <div class="stars"> | ||
| <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> |
There was a problem hiding this comment.
There are two nested elements with the class stars which seems incorrect. You should have only one block element and multiple stars__star elements inside it to represent individual stars. Check the BEM structure to ensure the block is not nested within itself.
| <div class="card__price-text">Price:</div> | ||
| <div class="card__price-tag">$2,199</div> | ||
| </div> | ||
| <a |
There was a problem hiding this comment.
The href attribute is missing in the anchor tag. If this button is meant to link to a product or a purchase page, you should include the appropriate URL in the href attribute.
| } | ||
|
|
||
| &__content { | ||
| font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
According to the checklist's code style rules, font-family should be defined in a variable for reusability and to ensure consistency across the project. Consider creating a variable for the font-family.
| font-weight: 500; | ||
| font-size: 12px; | ||
| line-height: 18px; | ||
| color: #060b35; | ||
| margin-top: 0; | ||
| margin-bottom: 4px; |
There was a problem hiding this comment.
The values for font-size and line-height should also be extracted into variables. This will make it easier to maintain consistent typography throughout the application.
| font-weight: 400; | ||
| font-size: 10px; | ||
| line-height: 14px; | ||
| color: #616070; | ||
| margin-top: 0; | ||
| margin-bottom: 16px; |
There was a problem hiding this comment.
Similar to the previous comment, font-size and line-height values should be stored in variables for maintainability and ease of changes in the future.
| font-weight: 700; | ||
| font-size: 16px; | ||
| line-height: 18px; | ||
| color: #060b35; |
There was a problem hiding this comment.
Consider using a variable for the font-size and line-height to maintain consistency and to make the codebase easier to maintain.
| .stars { | ||
| display: flex; | ||
| height: 16px; | ||
| justify-content: space-between; |
There was a problem hiding this comment.
According to the Code Style Rules, properties should be ordered alphabetically. Please rearrange the properties of the .stars selector.
| margin-bottom: 24px; | ||
|
|
||
| &--4 { | ||
| gap: 4px; |
There was a problem hiding this comment.
The gap property should be defined within the .stars block, not only for the .stars--4 modifier. This is because all star blocks likely need a gap, and we should keep the styles consistent.
| background-repeat: no-repeat; | ||
| width: 16px; | ||
| height: 16px; | ||
| background-position: center; |
There was a problem hiding this comment.
The background-position property should be combined with the background shorthand property used in lines 12 and 24 for better code organization.
DEMO LINK
TEST REPORT LINK
there 2 BEM blocks
cardandstarseach in it own fileSCSS Nesting is used for
elements,modifiersandpseudo-classesSCSS Variables are used for main values and placed in a separate file
all
stars--Nmodifiers work as expected (Highlight firstNstars)Code follows all the Code Style Rules ❗️