Develop#6889
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a couple of core checklist and requirement violations that block full completion of the task. First, the active (yellow) stars are currently implemented by changing the background-image directly on .stars__star for modifiers like stars--1…stars--5; the checklist explicitly requires that yellow stars be rendered via a pseudo-element (such as ::before) controlled by the container modifier, so you need to move this logic into a pseudo-selector and use .stars--N .stars__star:nth-child(-n + N)::before { ... } instead [CHECKLIST ITEM #1]. Second, the stars--0 state is not explicitly enforced: there is no dedicated rule that guarantees all stars remain inactive when the container has stars--0, but the requirement states that for each stars--N block, exactly the first N stars are active, so you should add a .stars--0 rule (e.g. making sure no pseudo-element shows yellow). In addition, you are missing a font-family with at least one fallback font anywhere in your CSS, which is required by the code style checklist; for example, you can set body { font-family: system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', sans-serif; } to resolve this. These changes are not cosmetic: they directly relate to the specified behavior and required implementation pattern, so they must be addressed before the solution can be considered complete. Once you add the pseudo-element-based yellow stars, explicitly handle stars--0, and define a fallback font-family, your work will align well with the task’s requirements and checklist.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| margin-right: 0; | ||
| } | ||
|
|
||
| .stars--1 .stars__star:nth-child(-n + 1) { |
There was a problem hiding this comment.
The requirements state that the block with stars--N modifier should control active stars via the modifier; you currently have selectors for stars--1 through stars--5 but nothing for stars--0, so stars--0 doesn’t explicitly enforce “0 active stars” via its modifier.
| @@ -0,0 +1,40 @@ | |||
| .stars { | |||
| display: flex; | |||
| } | |||
There was a problem hiding this comment.
Checklist item "Remember to use fallback fonts - alternative font-family" is not satisfied: there is no font-family defined here at all (and no fallback). Add a font-family with at least one fallback font.
| <meta | ||
| name="viewport" | ||
| content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0" | ||
| content="width=device-width, initial-scale=1.0" | ||
| /> |
There was a problem hiding this comment.
This uses background-image directly on the element and different URLs per modifier, but checklist item #1 requires "Yellow stars are added with container modifier + pseudo-selector (NO extra classes)", so you should move the active star rendering into a pseudo-element controlled by the .stars--N modifier instead of swapping the element background here.
| </div> | ||
|
|
There was a problem hiding this comment.
The active star state is set by changing background-image on the base element for .stars--1; this violates checklist item #1: "Yellow stars are added with container modifier + pseudo-selector (NO extra classes)". Use a pseudo-element (e.g. ::before) combined with the container modifier instead.
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> |
There was a problem hiding this comment.
Same issue as for .stars--1: active stars should be controlled via a pseudo-element, not by changing the background-image of .stars__star directly, to comply with checklist item #1.
| </div> | ||
|
|
There was a problem hiding this comment.
Same pseudo-selector requirement applies here; .stars--3 should not change the element background directly, but rather control a pseudo-element for yellow stars.
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> |
There was a problem hiding this comment.
Again, this repeats the pattern that violates checklist item #1 (no pseudo-selector used for yellow stars). Consider consolidating logic with a pseudo-element.
| </div> | ||
|
|
There was a problem hiding this comment.
The .stars--5 rule also sets the active state by swapping background-image instead of using a pseudo-element combined with the container modifier, which conflicts with checklist item #1.
| @@ -1 +1,5 @@ | |||
| /* add styles here */ | |||
| @import './blocks/stars/stars.css'; | |||
There was a problem hiding this comment.
This violates the requirement: "The block with stars--N modifier should have exactly N first stars active" because there is no explicit rule for stars--0 to guarantee that all stars stay inactive; consider adding a .stars--0 .stars__star { background-image: url('../../images/star.svg'); } rule so the 0‑rating state is clearly defined.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved — the implementation now cleanly matches the task requirements and the checklist. You correctly reset the default page margin on body, created six stars blocks with exactly five stars__star elements each, and applied the stars--0 through stars--5 modifiers so that the first N stars are active for each block as required [CHECKLIST ITEM #1]. The stars are rendered via background-image without using <img> or <svg>, and the active (yellow) state is nicely handled through a ::before pseudo-element driven by the container modifiers, which is exactly the intended pattern. Your BEM structure is solid: a dedicated stars.css file for the block, imported from style.css, and no extra classes or tag selectors beyond body, satisfying the BEM and code-style expectations [CHECKLIST ITEMS #2 and #3]. Layout-wise, using display: flex on the stars block without gap, spacing via margins on the elements, and no vertical margins between blocks all follow the constraints very well. You also added a proper font-family with fallbacks, which completes the remaining style requirements and shows good attention to detail. Overall, this is a clean, focused solution that respects both the visual and structural guidelines—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Typical MistakesfromBEMlesson theory are checked.