Skip to content

add cards solution#6522

Open
onthelonesomeroad wants to merge 3 commits intomate-academy:masterfrom
onthelonesomeroad:develop
Open

add cards solution#6522
onthelonesomeroad wants to merge 3 commits intomate-academy:masterfrom
onthelonesomeroad:develop

Conversation

@onthelonesomeroad
Copy link
Copy Markdown

  • DEMO LINK

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

  • there 2 BEM blocks card and stars each in their own file

  • SCSS Nesting is used for elements, modifiers and pseudo-classes

  • SCSS Variables are used for main values and placed in a separate file

  • all stars--N modifiers work as expected (Highlight first N stars)

  • Code follows all the Code Style Rules ❗️

Copy link
Copy Markdown

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

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 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/styles/card.scss
Comment on lines +1 to +16
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;
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 #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.

Comment thread src/styles/card.scss
Comment on lines +47 to +53
color: rgba(97, 96, 112, 1);
text-align: left;
width: 100%;

margin-bottom: 16px;
}

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 #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).

Comment thread src/styles/card.scss
Comment on lines +1 to +16
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;
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 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'); } } }

Comment thread src/styles/stars.scss
Comment on lines +1 to +16
.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);
}
}
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: "- [ ] 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.

Copy link
Copy Markdown

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

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 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/styles/stars.scss
Comment on lines +1 to +16
.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);
}
}
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 the checklist items:

  • "SCSS Nesting is used for elements, modifiers and pseudo-classes"
  • "all stars--N modifiers work as expected (Highlight first N stars)"

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.

Comment thread src/styles/card.scss
Comment on lines +13 to +15
border: 1px solid rgba(243, 243, 243, 1);
border-radius: 5px;

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 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.

Comment thread src/index.html
<link
rel="preconnect"
href="https://fonts.googleapis.com"
/>
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: "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).

Copy link
Copy Markdown

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

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 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! ✨

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