Skip to content

test completed#6891

Open
Panini9966 wants to merge 4 commits into
mate-academy:masterfrom
Panini9966:master
Open

test completed#6891
Panini9966 wants to merge 4 commits into
mate-academy:masterfrom
Panini9966:master

Conversation

@Panini9966

Copy link
Copy Markdown

Task " stars "

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one core checklist requirement is not met: each BEM block must have its own separate CSS file. Right now, your .stars block styles and the global body styles are all in src/style.css, which directly violates the requirement “Each BEM block has its own separate file” [CHECKLIST ITEM #2]. Functionally, your solution is otherwise very strong: the BEM structure is correct, modifiers stars--0 through stars--5 work as intended, stars are activated via container modifiers with nth-child selectors, and you correctly use background-image and display: flex without gap.

To fix the blocking issue, move all .stars-related rules (e.g. .stars, .stars__star, and modifier selectors like .stars--3 .stars__star:nth-child(-n + 3)) from src/style.css into a dedicated file such as src/stars.css. Then, either link stars.css separately in index.html or import it from your main CSS file, keeping only truly global/base rules (like body { margin: 0; }) in style.css. Once you split the styles this way, your implementation should fully satisfy both the functional requirements and the BEM/checklist rules. Overall, you’re very close; this is mainly a structural adjustment to align with the required project organization.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/style.css Outdated
margin: 0;
}

.stars {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item Each BEM block has its own separate file because the .stars block styles are defined here together with global body styles in one style.css instead of being placed in a dedicated file for the stars BEM block.

@Panini9966 Panini9966 left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes After AI reqview

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there is still a structural violation of the BEM file-organization rule, which is considered a core checklist requirement. All styles for the stars block (block, elements, and modifiers) must live in a single dedicated file, but right now .stars__star and .stars__star:last-child remain in src/style.css while the rest of the stars styles are in src/stylestars.css [CHECKLIST ITEM #2]. This means the stars block is split across multiple files, which does not meet the stated requirement that each BEM block has its own separate file.

To fix this, move the rules for .stars__star and .stars__star:last-child from src/style.css into src/stylestars.css, keeping style.css only for global/base styles like body { margin: 0; }. Once that’s done, the implementation will align with both the behavioral requirements (which you already satisfy) and the BEM/file-structure checklist. Aside from this file-organization point, your stars behavior, modifiers, and use of background-image and flexbox all look correct, so you’re very close to a fully compliant solution. Keep going—this is a small but important structural adjustment that will strengthen your understanding of BEM organization.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/stylestars.css
.stars--4 .stars__star:nth-child(-n + 4),
.stars--5 .stars__star:nth-child(-n + 5) {
background-image: url(images/star-active.svg);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file still contains .stars__star and .stars__star:last-child rules, which belong to the stars BEM block. This violates checklist item #2: "Each BEM block has its own separate file"; move all stars-related styles into the dedicated block CSS (here stylestars.css), keeping style.css only for global/base styles like body.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your solution is approved and meets the key functional and checklist requirements for this task. You correctly created six .stars blocks, each with five .stars__star elements, and used modifiers stars--0 through stars--5 so that the first N stars become active purely via modifier + nth-child selectors, which matches the requirement that no extra classes are added to the stars themselves. The use of display: flex on .stars, avoiding gap, and implementing the stars via background-image instead of <img>/<svg> is all done properly and cleanly. You also followed the BEM/file-organization rule by putting all stars block styles into stylestars.css and keeping style.css for global styles like body { margin: 0; }, which is exactly what the checklist expects.

One small improvement for future projects is to add a fallback font stack on body, for example font-family: system-ui, -apple-system, BlinkMacSystemFont, sans-serif;, to align with general code style recommendations. Overall, the structure, naming, and CSS are neat and focused, and you’ve applied the constraints (no * selector, no tag selectors beyond body, no extra margins between blocks) very well. Nicely done—this is solid, clean work that shows good attention to both functionality and the given rules.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

@etojeDenys etojeDenys left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the demo and test links do not work. please fix them and re-request the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants