add cards solution#6522
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because two critical checklist requirements for the stars block and SCSS structure are not yet met, which will cause the automated checks for this task to fail. You implemented BEM, variables, flexbox layout, Google Fonts, and hover states correctly, so the overall structure is solid, but the rating behavior is incomplete.
The main blocking issue is that there are no .stars--N modifiers in either your HTML or src/styles/stars.scss, so you cannot highlight the first N stars as required by the task [CHECKLIST ITEM #4]. In index.html, your stars container is currently just class="stars"; you need to change it (for example) to class="stars stars--4" to represent a 4‑star rating, and similarly for other ratings. In src/styles/stars.scss, the base .stars__star currently hardcodes active stars and only makes the last one inactive; instead, set all stars to the inactive image by default, then add nested modifier rules under .stars such as:
&--3 { &__star:nth-child(-n+3) { /* active styles (active star image) */ } }
so that each .stars--N modifier highlights the first N stars and leaves the rest inactive. This will also satisfy the requirement to use SCSS nesting for modifiers [CHECKLIST ITEM #2]; if needed, you can add similar &--... nested rules to your card block to demonstrate modifier nesting there as well. Once you implement these modifier classes and switch the logic so the default is inactive stars overridden by .stars--N, your solution will be functionally complete and aligned with 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
| body { | ||
| margin: 0; | ||
| font-family: Roboto, sans-serif; | ||
| } | ||
|
|
||
| .card { | ||
| display: flex; | ||
| box-sizing: border-box; | ||
| flex-direction: column; | ||
| width: $card-width; | ||
| height: $card-height; | ||
|
|
||
| border: 1px solid rgba(243, 243, 243, 1); | ||
| border-radius: 5px; | ||
|
|
||
| padding-top: 32px; |
There was a problem hiding this comment.
This violates checklist item #2: "SCSS Nesting is used for elements, modifiers and pseudo-classes". I can see nesting is used for elements and a pseudo-class (e.g. &__button:hover), but there are no nested rules for modifiers anywhere (e.g. no &--N rules). Add nested modifier rules in your SCSS (for example: .stars { &--3 { &__star:nth-child(-n+3) { /* active styles */ } } }) so modifiers are authored via SCSS nesting.
| color: rgba(97, 96, 112, 1); | ||
| text-align: left; | ||
| width: 100%; | ||
|
|
||
| margin-bottom: 16px; | ||
| } | ||
|
|
There was a problem hiding this comment.
This violates checklist item #4: "all stars--N modifiers work as expected (Highlight first N stars)". The HTML markup for the stars block has only <div class="stars"> and the five star elements, but there are no stars--N modifier classes applied (e.g. class="stars stars--4"). You need to add modifier classes in HTML and implement corresponding styles in stars.scss so the first N stars are highlighted (for example, use .stars--N .stars__star:nth-child(-n+N) to set the active background-image).
| body { | ||
| margin: 0; | ||
| font-family: Roboto, sans-serif; | ||
| } | ||
|
|
||
| .card { | ||
| display: flex; | ||
| box-sizing: border-box; | ||
| flex-direction: column; | ||
| width: $card-width; | ||
| height: $card-height; | ||
|
|
||
| border: 1px solid rgba(243, 243, 243, 1); | ||
| border-radius: 5px; | ||
|
|
||
| padding-top: 32px; |
There was a problem hiding this comment.
The current stars.scss hardcodes an active star for every .stars__star and swaps the last child to an inactive image via &__star:last-child. This approach doesn't support arbitrary stars--N modifiers. Update stars.scss to provide inactive default for all stars and add modifier rules (nested) to apply the active image to the first N stars, e.g.:
.stars { &__star { background-image: url('../images/star.svg'); } &--3 { &__star:nth-child(-n+3) { background-image: url('../images/star-active.svg'); } } }
| .stars { | ||
| display: flex; | ||
|
|
||
| &__star { | ||
| background-image: url('../images/star-active.svg'); | ||
| width: 16px; | ||
| height: 16px; | ||
| background-position: center; | ||
| background-repeat: no-repeat; | ||
| margin-right: 4px; | ||
| } | ||
|
|
||
| &__star:last-child { | ||
| background-image: url(../images/star.svg); | ||
| } | ||
| } |
There was a problem hiding this comment.
This violates checklist item: "- [ ] SCSS Nesting is used for elements, modifiers and pseudo-classes"
In stars.scss there are no modifier rules (e.g. .stars--1, .stars--2, ...). You must implement modifier selectors (nested under .stars) that use :nth-child() (or similar) to highlight the first N stars and set the rest to inactive. For example, create nested rules like &--3 { .stars__star:nth-child(-n+3) { ...active... } .stars__star:nth-child(n+4) { ...inactive... } } so modifiers are implemented using SCSS nesting.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a core functional requirement for the stars component is not yet met, and this will cause the task checks to fail. The checklist explicitly requires that all stars--N modifiers (for N = 1..5) correctly highlight the first N stars, but in src/styles/stars.scss you currently only implement behavior for .stars--4, so ratings like .stars--1, .stars--2, .stars--3, and .stars--5 do not work as specified. To fix this, keep your base .stars__star rule as the inactive image, then under the .stars block add nested modifier rules &--1 through &--5, each with a nested &__star:nth-child(-n+N) that switches those first N stars to the active image (e.g., &--3 { &__star:nth-child(-n+3) { background-image: url('../images/star-active.svg'); } }). This will both satisfy the functional requirement (all ratings highlight correctly) and the SCSS nesting requirement for modifiers. As a minor improvement, you can also make your url() paths consistent by using the same quoting style for all background images, but this is not a blocker. Aside from the stars modifiers, the rest of the card layout, BEM structure, use of variables, fonts, and data-qa attributes looks solid, so once you adjust the stars SCSS you should be in good shape. Keep going—this is a focused change and will complete an otherwise strong 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
| .stars { | ||
| display: flex; | ||
|
|
||
| &__star { | ||
| background-image: url('../images/star.svg'); | ||
| width: 16px; | ||
| height: 16px; | ||
| background-position: center; | ||
| background-repeat: no-repeat; | ||
| margin-right: 4px; | ||
| } | ||
|
|
||
| &--4 &__star:nth-child(-n + 4) { | ||
| background-image: url(../images/star-active.svg); | ||
| } | ||
| } |
There was a problem hiding this comment.
This violates the checklist items:
- "SCSS Nesting is used for
elements,modifiersandpseudo-classes" - "all
stars--Nmodifiers work as expected (Highlight firstNstars)"
You currently have only a .stars--4 rule (&--4 &__star:nth-child(-n + 4)) so only the 4-star case works. Also the modifier is not defined using nested modifier block form. Change the stars SCSS so all modifiers work and use nesting, for example:
.stars {
&__star { /* default inactive image */ }
&--1 { &__star:nth-child(-n+1) { /* active / } }
&--2 { &__star:nth-child(-n+2) { / active / } }
/ ... up to &--5 */
}
This ensures the first N stars are highlighted and satisfies the nesting requirement.
| border: 1px solid rgba(243, 243, 243, 1); | ||
| border-radius: 5px; | ||
|
|
There was a problem hiding this comment.
This doesn't satisfy the checklist item: "all stars--N modifiers work as expected (Highlight first N stars)". Your stars.scss defines only &--4 behavior, so other stars--N modifiers (e.g., --1, --2, --3, --5) are missing. Implement modifiers for all needed ratings (1..5) using SCSS nesting, for example:
.stars {
&__star { background-image: url('../images/star.svg'); }
&--3 { &__star:nth-child(-n+3) { background-image: url('../images/star-active.svg'); } }
}
This ensures the first N stars are highlighted while the rest remain inactive.
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.googleapis.com" | ||
| /> |
There was a problem hiding this comment.
This violates checklist item: "all stars--N modifiers work as expected (Highlight first N stars)" — in src/styles/stars.scss you only define the .stars--4 modifier. You need modifiers for all required ratings (for example &--1, &--2, &--3, &--4, &--5) so each .stars--N highlights exactly the first N stars. Implement them using SCSS nesting, e.g.: .stars { &--3 { &__star:nth-child(-n+3) { background-image: url('../images/star-active.svg'); } } }. Also ensure the base .stars__star stays the inactive image (as you already have).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved and it clearly meets the main requirements for layout, SCSS structure with separate BEM blocks, variables usage, and fully working stars--1 through stars--5 modifiers. There are no critical problems; the remaining points are minor, so this is an educational approval to help you keep progressing while you learn.
One improvement to consider in src/styles/card.scss is that object-fit: contain is currently applied to the .card__image container, but it should be applied to the actual image element (your .card__media <img>) so that the picture scales and crops as intended. In src/styles/stars.scss, your background-image URLs work correctly, but the quoting style is inconsistent (one url() uses quotes and the other does not); choosing one style and using it everywhere will make the code cleaner and easier to read. You might also consider nesting each modifier’s logic inside its own block (for example, &--3 { &__star:nth-child(-n + 3) { ... } }) to make the relationship between modifiers and highlighted stars even clearer, though your current grouped selectors are already functionally correct.
Overall, you’ve done a solid job implementing the checklist items, especially the SCSS structure, hover states, and the dynamic stars behavior—nice work, and keep building on this foundation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
DEMO LINK
TEST REPORT LINK
❗️ Copy this
Checklistto thePull Requestdescription after links, and put- [x]before each point after you checked it.there 2 BEM blocks
cardandstarseach in their own fileSCSS Nesting is used for
elements,modifiersandpseudo-classesSCSS Variables are used for main values and placed in a separate file
all
stars--Nmodifiers work as expected (Highlight firstNstars)Code follows all the Code Style Rules ❗️