Skip to content

develop#6885

Open
tkrylataja wants to merge 2 commits into
mate-academy:masterfrom
tkrylataja:develop
Open

develop#6885
tkrylataja wants to merge 2 commits into
mate-academy:masterfrom
tkrylataja:develop

Conversation

@tkrylataja

Copy link
Copy Markdown

❗️ Copy this Checklist to the Pull Request description after links, and put - [x] before each point after you checked it.

  • Yellow stars are added with container modifier + pseudo-selector (NO extra classes)
  • Each BEM block has its own separate file
  • All Typical Mistakes from BEM lesson theory are checked.
  • Code follows all the Code Style Rules ❗️

@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 are still checklist-level requirements not fully met, even though the core stars functionality works correctly. First, per [CHECKLIST ITEM #2], each BEM block must have its own CSS file, but your stars block styles are currently inside src/style.css; please move all .stars and .stars__star rules into a dedicated file like src/stars.css and include that file from index.html (e.g., with an additional <link> tag). Second, the rule src/style.css: .stars:not(:last-child) { margin-bottom: 0; } conflicts with the requirement “Don’t add vertical margins between blocks” and is unnecessary because blocks already have no vertical margin by default; remove this selector entirely to avoid confusion in layout behavior. These changes are both required by the task description and checklist, so they need to be fixed before the solution can be considered complete. Once you separate the stars block into its own CSS file and clean up the redundant margin rule, your implementation will align well with the stated standards for this task. Overall, your functional implementation of the stars block is solid, and these adjustments will help you fully match the BEM and checklist expectations.


✨ 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
padding: 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.

The project requirements and checklist item #2 state "Each BEM block has its own separate file", but all styles are placed in a single style.css. You should move the stars block styles into their own dedicated CSS file and include it separately.

Comment thread src/style.css Outdated
display: flex;
}

.stars:not(:last-child) {

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 rule is unnecessary and conflicts with the requirement "Don't add vertical margins between blocks." Since you already don't set any vertical margins on .stars, consider removing this selector entirely to avoid confusion.

@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 because it fully meets the functional and checklist requirements for this task. You correctly reset the browser default margins in style.css, and index.html contains exactly six stars blocks with five stars__star elements each, using the stars--0 through stars--5 modifiers as required. The active yellow stars are handled purely through container modifiers and nth-child selectors, without extra classes, which matches [CHECKLIST ITEM #1]. The stars are implemented via background-image on span elements (no <img> or inline <svg>), and the stars container uses display: flex without the gap property. There are no vertical margins added between the blocks, and the spacing between stars is achieved correctly using right margins. You also separated the BEM block styles into stars.css and linked it from index.html, satisfying the block separation requirement [CHECKLIST ITEM #2]. Overall, the structure, BEM usage, and visual behavior are consistent with the task description, so no changes are needed. Nice work bringing everything in line with the requirements.


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

Footnotes

  1. Rate AI review example

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.

2 participants