Develop#6526
Conversation
Vladislav-Riabovol
commented
Apr 20, 2026
- DEMO LINK
- TEST REPORT LINK
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this can be considered complete, mainly around the stars component and image paths.
The most important issue is that the stars block does not correctly support modifier classes like stars--N to highlight the first N stars: in src/styles/blocks/stars.scss (and any related rules in card.scss) you currently hardcode :nth-child(-n + 4), which always activates 4 stars; instead, create nested modifier rules such as:
.stars { &--3 { &__star:nth-child(-n + 3) { /* active styles */ } } &--4 { &__star:nth-child(-n + 4) { ... } } }
so the HTML (<div class="stars stars--3">etc.) controls how many stars are active.
Related to that, the checklist requires SCSS nesting for modifiers, so be sure the modifier selectors (&--3,&--4, etc.) are nested inside the.starsblock rather than written as separate top-level selectors.
Another critical issue is that yourbackground-imageURLs for the stars (insrc/styles/blocks/stars.scss,src/styles/blocks/card.scss, and any inline styles insrc/index.html) use absolute paths like/src/images/star.svg; these must be paths relative tosrc/styles/index.scss, for examplebackground-image: url('../images/star.svg')andurl('../images/star-active.svg').
As a smaller improvement, your PR description should also include the filled-out checklist after the demo and test links, marked with- [x]for completed items, as required by the task instructions.
You’ve done a solid job with BEM structure, SCSS variables, flex layout, hover states, and data-qa attributes, so once you adjust the star modifiers and image URLs, your solution will align well with the full set of requirements.
✨ 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
| @@ -6,13 +6,66 @@ | |||
| name="viewport" | |||
| content="width=device-width, initial-scale=1.0" | |||
| /> | |||
There was a problem hiding this comment.
This uses an absolute path for the star image. The task requires: "background-image: url() should be relative to the index.scss. So it should start with ../images." Change to something like: background-image: url('../images/star.svg'); (index.scss is in src/styles).
| rel="preconnect" | ||
| href="https://fonts.googleapis.com" | ||
| /> | ||
| <link |
There was a problem hiding this comment.
This uses an absolute path for the active star image. The task requires: "background-image: url() should be relative to the index.scss. So it should start with ../images." Change to something like: background-image: url('../images/star-active.svg');.
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.googleapis.com" | ||
| /> |
There was a problem hiding this comment.
This selector hardcodes highlighting the first 4 stars. The checklist requires: "all stars--N modifiers work as expected (Highlight first N stars)" — your implementation should support modifiers like .stars--3, .stars--4 etc. Consider adding modifier rules (nested in .stars) such as .stars--3 &__star:nth-child(-n + 3) or nesting .stars--3 { &__star:nth-child(-n + 3) { ... } } so the number of highlighted stars is controlled by the modifier class.
| @@ -0,0 +1,16 @@ | |||
| .stars { | |||
There was a problem hiding this comment.
This violates checklist item: 'SCSS Nesting is used for elements, modifiers and pseudo-classes'. The stars block does not define or nest any BEM modifiers. Add nested modifier rules (for example .stars--4 { .stars__star:nth-child(-n + 4) { ... } }) so modifiers are implemented using SCSS nesting.
| &__star { | ||
| width: 16px; | ||
| height: 16px; | ||
| background-image: url('/src/images/star.svg'); |
There was a problem hiding this comment.
This violates requirement: 'background-image: url() should be relative to the index.scss. So it should start with ../images.' Currently the file uses url('/src/images/star.svg'). Change it to a path relative to src/styles/index.scss, e.g. background-image: url('../images/star.svg').
| &:nth-child(-n + 4) { | ||
| background-image: url('/src/images/star-active.svg'); |
There was a problem hiding this comment.
This violates checklist item: 'all stars--N modifiers work as expected (Highlight first N stars)'. The code currently hardcodes &:nth-child(-n + 4) which always highlights the first 4 stars. Implement BEM modifiers like .stars--3, .stars--4, etc., and move the nth-child rule inside those modifier blocks so the HTML can control how many stars are highlighted (e.g. <div class="stars stars--4">).
| border: 1px solid $color-border; | ||
|
|
||
| &__image { | ||
| width: 160px; |
There was a problem hiding this comment.
This line uses absolute paths '/src/images/...'. The task requires: "background-image: url() should be relative to the index.scss. So it should start with ../images." Change to a relative path (for example: url('../images/star.svg')).
| } | ||
|
|
There was a problem hiding this comment.
Here you hardcode active stars with &:nth-child(-n + 4) which highlights the first 4 stars only. This violates the requirement: "all stars--N modifiers work as expected (Highlight first N stars)". Implement modifier-based rules (for example .stars--4 / .stars--3, etc.) and use SCSS nesting so the modifier controls which first N .stars__star get the active background-image.
| body { | ||
| margin: 0; | ||
| } | ||
| @import 'utils/variables'; |
There was a problem hiding this comment.
This violates the checklist requirement: "SCSS Nesting is used for elements, modifiers and pseudo-classes" because modifiers are not implemented using nesting in the stars block. Add nested modifier rules (for example .stars--4 { &__star:nth-child(-n + 4) { ... } }) to satisfy the requirement that modifiers are expressed via SCSS nesting.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed because a key functional requirement for the stars component and one path requirement are not yet met.
The main blocker is the stars modifiers: in src/styles/blocks/stars.scss (and mirrored usage in card.scss), the modifier selectors like &--3 / &--4 are currently nested inside &__star, which produces selectors that do not match HTML such as <div class="stars stars--3">. According to the task and checklist ([CHECKLIST ITEM #6], [CHECKLIST ITEM #20], [CHECKLIST ITEM #28], [CHECKLIST ITEM #29]), modifiers must be nested directly under .stars, for example:
.stars {
&__star { /* base star styles */ }
&--3 {
&__star:nth-child(-n + 3) {
/* active star styles */
}
}
&--4 {
&__star:nth-child(-n + 4) {
/* active star styles */
}
}
}This way, a class like class="stars stars--3" correctly highlights the first 3 stars. Please restructure your modifiers this way (ideally covering all needed stars--N values) so that each modifier controls exactly the first N stars.
The second critical issue is the product image path in src/index.html: the <img> currently uses an absolute source like src="/src/images/imac.jpeg", which violates the requirement to avoid leading /src/... paths and to use paths consistent with your SCSS background images. Update this to a relative path such as src="images/imac.jpeg" or src="./images/imac.jpeg" so it works when deployed from the project root. As a smaller process improvement, the PR description is missing the copied checklist with - [x] marks as required; next time, please include and fill out that checklist so reviewers can quickly verify each requirement. Overall, your card layout, BEM structure, variables, fonts, and hover states look good—once you fix the stars nesting and the image path, this implementation should fully satisfy the task requirements.
✨ 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
| border: 1px solid $color-border; | ||
|
|
||
| &__image { | ||
| width: 160px; | ||
| display: block; | ||
| height: 134px; | ||
| margin: 0 auto 40px; | ||
| } | ||
|
|
||
| &__title { | ||
| font-size: 12px; | ||
| font-weight: 500; | ||
| line-height: 18px; | ||
| margin: 0 0 4px; | ||
| color: $color-dark; | ||
| } | ||
|
|
||
| &__code { | ||
| font-size: 10px; |
There was a problem hiding this comment.
This violates the checklist item: "- [ ] all stars--N modifiers work as expected (Highlight first N stars)"
The stars modifiers are implemented incorrectly: the modifier rules (&--3, &--4) are nested inside &__star rather than inside the .stars block. As written they generate selectors that don't match HTML like <div class="stars stars--3">. Move modifiers to be nested under .stars (same level as &__star) and inside each modifier target &__star:nth-child(-n + N) so .stars--3 highlights the first 3 stars.
| .card { | ||
| box-sizing: border-box; | ||
| width: 200px; | ||
| padding: 32px 16px 16px; | ||
| border: 1px solid $color-border; | ||
|
|
||
| &__image { | ||
| width: 160px; | ||
| display: block; | ||
| height: 134px; | ||
| margin: 0 auto 40px; | ||
| } | ||
|
|
||
| &__title { | ||
| font-size: 12px; | ||
| font-weight: 500; | ||
| line-height: 18px; | ||
| margin: 0 0 4px; | ||
| color: $color-dark; | ||
| } | ||
|
|
||
| &__code { | ||
| font-size: 10px; |
There was a problem hiding this comment.
This violates the checklist item: "- [ ] SCSS Nesting is used for elements, modifiers and pseudo-classes"
Right now the modifier selectors are nested in the wrong place. From stars.scss you should have the modifier nesting inside .stars, for example:
.stars { &__star { ... } &--3 { &__star:nth-child(-n + 3) { /* active */ } } }
Currently &--3 is written inside &__star, which breaks the required modifier nesting structure.
| } | ||
|
|
||
| &__title { | ||
| font-size: 12px; | ||
| font-weight: 500; | ||
| line-height: 18px; | ||
| margin: 0 0 4px; | ||
| color: $color-dark; | ||
| } |
There was a problem hiding this comment.
This violates the previous required correction: "Fix the stars block so that modifier classes like stars--N correctly control how many stars are active instead of always activating 4 stars."
In stars.scss you currently nest modifier rules inside &__star and use selectors like &__star:nth-child(-n + 3) in the wrong scope. That causes the modifiers in HTML (<div class="stars stars--3">) not to control which stars become active. Nest the modifiers inside .stars and target &__star:nth-child(-n + N) there.
| width: 166px; | ||
| margin-bottom: 24px; | ||
| } | ||
|
|
There was a problem hiding this comment.
This violates the previous required correction about image paths: "Update background-image URLs for stars and card images to be relative to src/styles/index.scss, e.g. background-image: url('../images/star.svg'), instead of using absolute paths like /src/images/star.svg."
The stars background-image paths are correct (use ../images/...), but the card image in index.html is using an absolute path: src="/src/images/imac.jpeg". Change the <img> path to a relative path (for example src="images/imac.jpeg" or src="./images/imac.jpeg") so it does not rely on the leading /src/... absolute path.
| /> | ||
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.gstatic.com" |
There was a problem hiding this comment.
The nested :nth-child(-n + N) selectors are currently placed under the wrong parent. For example at this line you have &__star:nth-child(-n + 3) inside &--3 which itself is inside &__star — that will not target the stars as intended. Move &--3 / &--4 out of the &__star block and place them inside .stars, then use &__star:nth-child(-n + 3) (or 4) inside those modifiers so the HTML modifier stars--3 activates the first 3 stars.
| &--3 { | ||
| &__star:nth-child(-n + 3) { | ||
| background-image: url('../images/star-active.svg'); | ||
| } |
There was a problem hiding this comment.
This violates checklist item #28: "Fix the stars block so that modifier classes like stars--N correctly control how many stars are active instead of always activating 4 stars." The current SCSS nests the &--3 modifier inside &__star, producing incorrect selectors (for example .stars__star--3__star). Move modifier blocks so they are nested directly inside .stars and use &__star:nth-child(-n + 3) (or N) inside each &--N modifier so the HTML (<div class="stars stars--3">) controls the active stars.
| &__star { | ||
| width: 16px; | ||
| height: 16px; | ||
| background-image: url('../images/star.svg'); | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
|
|
||
| &--3 { | ||
| &__star:nth-child(-n + 3) { | ||
| background-image: url('../images/star-active.svg'); |
There was a problem hiding this comment.
This violates checklist item #29: "Ensure the modifier selectors for the stars block (e.g. &--3, &--4) are nested inside the .stars block rather than written as separate top‑level selectors." Also violates checklist item #6: "all stars--N modifiers work as expected (Highlight first N stars)". Currently &--3/&--4 are placed inside &__star. Restructure to something like:
.stars { &__star { ... } &--3 { &__star:nth-child(-n + 3) { background-image: url('../images/star-active.svg'); } } } and add modifiers for required N values.
| &--4 { | ||
| &__star:nth-child(-n + 4) { | ||
| background-image: url('../images/star-active.svg'); | ||
| } |
There was a problem hiding this comment.
This also violates checklist item #20: "SCSS Nesting is used for elements, modifiers and pseudo-classes" because modifiers are not nested in the correct place. The &--4 block (and any other &--N) should be placed inside the .stars block (not inside &__star) and apply &__star:nth-child(-n + 4) to mark the first 4 stars active. Consider providing modifiers for 1..5 so all possible stars--N behave correctly.