add task solution#6893
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core checklist requirement is not met: each BEM block must have its own separate file. Right now all styles, including the stars block (.stars, .stars__star, and all stars--N modifiers), are in src/style.css, which violates the structural requirement that every BEM block’s styles live in their own dedicated file ([CHECKLIST ITEM: "Each BEM block has its own separate file"]). To fix this, move all stars-related rules from src/style.css into a new block-specific file such as src/stars.css (or according to your project’s naming convention) and then include that file from index.html instead of or in addition to the global stylesheet. Make sure the global file only keeps truly global/page-level styles (like body { margin: 0; }) and that all block-specific logic for stars is isolated in the stars block file. Once that separation is in place and correctly wired in your HTML, the implementation should satisfy the structural checklist and be ready for approval, because the rest of the stars functionality and BEM usage looks solid.
✨ 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
| /* #region Stars modifiers */ | ||
| .stars--1 .stars__star:nth-child(1) { | ||
| background-image: url(./images/star-active.svg); | ||
| } | ||
|
|
||
| .stars--2 .stars__star:not(:nth-child(n + 3)) { | ||
| background-image: url(./images/star-active.svg); | ||
| } | ||
|
|
||
| .stars--3 .stars__star:not(:nth-child(n + 4)) { | ||
| background-image: url(./images/star-active.svg); | ||
| } | ||
|
|
||
| .stars--4 .stars__star:not(:nth-child(n + 5)) { | ||
| background-image: url(./images/star-active.svg); | ||
| } | ||
|
|
||
| .stars--5 .stars__star:not(:nth-child(n + 6)) { | ||
| background-image: url(./images/star-active.svg); | ||
| } | ||
|
|
||
| /* #endregion */ |
There was a problem hiding this comment.
This violates checklist item "Each BEM block has its own separate file". All styles for the stars block (base, elements, modifiers) are currently kept in a single global style.css. You need to move the stars block styles into their own dedicated file(s) according to the BEM/block separation requirement.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical blockers that break the required functionality of the stars component. First, in src/style.css the import path for the modifiers file is incorrect: it currently uses @import './blocks/stars modifiers.css'; with a space, but the actual filename is stars-modifiers.css, so the modifier rules for active stars are never loaded, and the requirement that stars--N has exactly N first stars active is not met. Second, src/blocks/stars-modifiers.css defines modifiers only for stars--1 through stars--5; per the task description you also need a stars--0 modifier that explicitly keeps all stars inactive, so that all six modifiers are handled consistently [CHECKLIST ITEM #1]. Finally, the build/check run shows a broken image path in src/blocks/stars__star.css at line 2: background-image: url(./images/star.svg); is resolving relative to src/blocks/, but the images folder is higher up, so you need to adjust this to the correct relative path (for example ../images/star.svg if images is under src/). These issues directly prevent the stars from rendering and behaving as specified, so they must be fixed before the solution can be considered complete. Once you correct the import, add the stars--0 rule, and fix the image path, the rest of your structure and use of BEM, flex, and pseudo-selectors looks solid—nice work getting close to the final solution.
✨ 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
| @@ -0,0 +1,19 @@ | |||
| .stars--1 .stars__star:nth-child(1) { | |||
There was a problem hiding this comment.
The task description requires modifiers stars--0, stars--1, stars--2, stars--3, stars--4, stars--5 "one per each", and that the block with stars--N has exactly N first stars active. This file defines rules only for stars--1–stars--5, so there is no explicit handling for stars--0. Consider adding a stars--0 rule (even if it just keeps all stars in the inactive state) to satisfy the requirement for all six modifiers.
| @import './blocks/body.css'; | ||
| @import './blocks/stars__star.css'; | ||
| @import './blocks/stars.css'; | ||
| @import './blocks/stars modifiers.css'; |
There was a problem hiding this comment.
The core functional requirement says: "The block with stars--N modifier should have exactly N first stars active" and checklist item #1 requires that yellow stars are added with container modifier + pseudo-selector. You have the correct stars--N rules in stars-modifiers.css, but this file is not imported because the path here uses a space ('./blocks/stars modifiers.css') instead of the actual filename stars-modifiers.css. Update this import to match the real file name so that the active-star styles are applied.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution under an educational override so you can move forward while continuing to learn from a few remaining details.
You’ve done a solid job overall: the stars block structure is correct, the modifiers stars--0 through stars--5 are implemented, and active yellow stars are controlled via container modifiers and selectors without extra classes, which aligns with [CHECKLIST ITEM #1]. There are two main areas to improve: first, your .stars--0 modifier in both src/blocks/stars-modifiers.css and src/blocks/stars.css still reassigns the inactive background-image to all stars, even though .stars__star already defines the default inactive icon—this goes against the idea that modifiers should only change what differs from the base state, so you should remove the background-image override from .stars--0 and let the base rule handle inactive stars. Second, in src/index.html all rating groups are plain div elements; the checklist asks you to "Use semantic tags where possible", so wrapping the groups in a section or article (depending on context) would better express the meaning of the ratings.
As minor improvements, remember to set a global font-family with a fallback (e.g., on body) to satisfy the font fallback requirement, and keep an eye on semantic structure elsewhere in the page. Overall, your component logic is working well, and these refinements will help you align even more closely with professional HTML/CSS practices. Keep going—this is a strong implementation, and polishing these details will further improve your skills.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| .stars--0 .stars__star { | ||
| background-image: url(../images/star.svg); |
There was a problem hiding this comment.
This violates checklist item #1: "Yellow stars are added with container modifier + pseudo-selector (NO extra classes)". Your base .stars__star already defines the default inactive star image, so stars--0 should not reassign background-image for all stars. Instead, let stars--0 be an empty modifier (or only adjust if needed) so that it simply keeps the default inactive state, and reserve background-image overrides in this file only for active (yellow) stars.
| @@ -0,0 +1,7 @@ | |||
| .stars { | |||
There was a problem hiding this comment.
This .stars--0 rule overrides the base .stars__star background-image with the same star.svg, which is redundant and goes against the idea that modifiers only toggle yellow (active) stars. Per the description and previous HIGH priority note, stars--0 should explicitly keep all stars inactive by not changing them to star-active.svg; you can simply omit changing background-image here and rely on .stars__star for the inactive state.
| </head> | ||
| <body> | ||
| <h1>Stars</h1> | ||
| <div class="stars stars--0"> |
There was a problem hiding this comment.
The checklist requires "Use semantic tags where possible". Right now all rating groups are plain div elements; consider using a semantic wrapper, such as a section or article (depending on context), or at least structuring the stars inside a meaningful semantic container to better comply with that guideline.
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 ❗️