Skip to content

Develop#6526

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

Develop#6526
Vladislav-Riabovol wants to merge 3 commits intomate-academy:masterfrom
Vladislav-Riabovol:develop

Conversation

@Vladislav-Riabovol
Copy link
Copy Markdown

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

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 .stars block rather than written as separate top-level selectors.
    Another critical issue is that your background-image URLs for the stars (in src/styles/blocks/stars.scss, src/styles/blocks/card.scss, and any inline styles in src/index.html) use absolute paths like /src/images/star.svg; these must be paths relative to src/styles/index.scss, for example background-image: url('../images/star.svg') and url('../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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.html
@@ -6,13 +6,66 @@
name="viewport"
content="width=device-width, initial-scale=1.0"
/>
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 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).

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

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

Comment thread src/styles/blocks/stars.scss Outdated
&__star {
width: 16px;
height: 16px;
background-image: url('/src/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 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').

Comment thread src/styles/blocks/stars.scss Outdated
Comment on lines +12 to +13
&:nth-child(-n + 4) {
background-image: url('/src/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 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;
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 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')).

Comment on lines +12 to +13
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread src/styles/index.scss
body {
margin: 0;
}
@import 'utils/variables';
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 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.

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

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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +5 to +23
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;
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 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.

Comment on lines +1 to +23
.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;
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 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.

Comment on lines +12 to +20
}

&__title {
font-size: 12px;
font-weight: 500;
line-height: 18px;
margin: 0 0 4px;
color: $color-dark;
}
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 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;
}

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

Comment thread src/index.html
Comment on lines +12 to +15
/>
<link
rel="preconnect"
href="https://fonts.gstatic.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.

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.

Comment on lines +12 to +15
&--3 {
&__star:nth-child(-n + 3) {
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 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.

Comment on lines +5 to +14
&__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');
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 #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.

Comment on lines +18 to +21
&--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 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.

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