Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this task! Your solution is approved as it meets all the core functional requirements. The HTML is well-structured, and the BEM implementation is excellent.
While your code works perfectly, there are a few opportunities to make the CSS more concise. In active-stars.css and stars.css, you have several repeated properties like padding and background-image across different modifier classes. You can refactor this by grouping selectors or moving common styles to the base class to avoid repetition. This practice, known as DRY (Don't Repeat Yourself), is key for writing maintainable code.
Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| margin-left: 4px; | ||
| margin-right: 4px; | ||
| } | ||
|
|
||
| .stars__star:first-child { | ||
| margin-left: 0; | ||
| } |
There was a problem hiding this comment.
Using both margin-left and margin-right to create space between flex items, combined with an override for the first child, is a bit complex. A cleaner approach is to apply a margin to only one side of the elements. For example, you could apply margin-left to all stars except the first one using the :not(:first-child) pseudo-class or the adjacent sibling selector (+). This would simplify the code and avoid adding unwanted space on the right side of the last star.
| .stars--0 { | ||
| padding-top: 2px; | ||
| padding-bottom: 2px; | ||
| } | ||
|
|
||
| .stars--1 { | ||
| padding-top: 2px; | ||
| padding-bottom: 2px; | ||
| } | ||
|
|
||
| .stars--2 { | ||
| padding-top: 2px; | ||
| padding-bottom: 2px; | ||
| } | ||
|
|
||
| .stars--3 { | ||
| padding-top: 2px; | ||
| padding-bottom: 2px; | ||
| } | ||
|
|
||
| .stars--4 { | ||
| padding-top: 2px; | ||
| padding-bottom: 2px; | ||
| } | ||
|
|
||
| .stars--5 { | ||
| padding-top: 2px; | ||
| padding-bottom: 2px; | ||
| } |
There was a problem hiding this comment.
These CSS rules are duplicated for all modifiers from --0 to --5. This code can be made more concise by applying the padding directly to the main .stars class, if it's needed for all star blocks.
However, the requirement states, "Don't add vertical margins between blocks." Adding vertical padding creates space between the blocks, which seems to go against the spirit of this requirement. Please review if this padding is necessary.
| .stars--1 .stars__star:nth-of-type(-n + 1) { | ||
| background-image: url('./../../images/star-active.svg'); | ||
| background-repeat: no-repeat; | ||
| } | ||
|
|
||
| .stars--2 .stars__star:nth-of-type(-n + 2) { | ||
| background-image: url('./../../images/star-active.svg'); | ||
| background-repeat: no-repeat; | ||
| } | ||
|
|
||
| .stars--3 .stars__star:nth-of-type(-n + 3) { | ||
| background-image: url('./../../images/star-active.svg'); | ||
| background-repeat: no-repeat; | ||
| } | ||
|
|
||
| .stars--4 .stars__star:nth-of-type(-n + 4) { | ||
| background-image: url('./../../images/star-active.svg'); | ||
| background-repeat: no-repeat; | ||
| } | ||
|
|
||
| .stars--5 .stars__star:nth-of-type(-n + 5) { | ||
| background-image: url('./../../images/star-active.svg'); | ||
| background-repeat: no-repeat; | ||
| } |
There was a problem hiding this comment.
These rules are quite repetitive. You can make your code more concise and maintainable by following the DRY (Don't Repeat Yourself) principle:
- The
background-repeat: no-repeat;property is redundant here because it's already defined for all stars instars.css. - Since the
background-imageis the same for all active stars, you can group all the selectors (from.stars--1to.stars--5) into a single rule to set this property just once.
2 participants
DEMO LINK
TEST REPORT LINK
Yellow stars are added with container modifier + pseudo-selector (NO extra classes)
Each BEM block has its own separate file
All
Typical MistakesfromBEMlesson theory are checked.Code follows all the Code Style Rules ❗️