Skip to content

add solution#6296

Open
lubovmrk wants to merge 3 commits into
mate-academy:masterfrom
lubovmrk:develop
Open

add solution#6296
lubovmrk wants to merge 3 commits into
mate-academy:masterfrom
lubovmrk:develop

Conversation

@lubovmrk

@lubovmrk lubovmrk commented Jul 2, 2026

Copy link
Copy Markdown
  • DEMO LINK

  • TEST REPORT LINK

  • All components follow BEM and use SCSS

  • Repeated sizes and special colors are put to variables

  • Grid is used for the columns

  • Cards are shown in 1, 2, 3, or 4 columns based on screen resolution

  • All changes on :hover are smooth

  • 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 a couple of critical requirement mismatches that block full task completion. Most notably, the catalog grid does not implement the required 1/2/3/4 column behavior at the explicit breakpoints (1 column <488px, 2 ≥488px, 3 ≥768px, 4 ≥1024px); using grid-template-columns: repeat(auto-fit, 200px); without media queries violates the responsive layout requirement [CHECKLIST ITEM #3, #4]. In addition, the star rating modifiers are wired incorrectly: the SCSS selectors like .stars__star--4 .stars__star:nth-child(-n + 4) do not match the actual HTML structure (class="stars stars__star--4" on the container), so the rating highlighting logic will not work as intended [CHECKLIST ITEM #6].

To fix the grid, add media queries on the catalog container (e.g. .main) that explicitly set grid-template-columns to 1, 2, 3, and 4 columns at the required widths instead of relying on auto-fit. For the stars, either move the modifier to a proper container-level class (e.g. class="stars stars--4") and use selectors like .stars--4 .stars__star:nth-child(-n + 4), or adjust the SCSS nesting so the modifier is applied to the correct element and the nth-child rules actually match. While not blocking, it would also be good to make card and button hover transitions explicit (e.g. transition: transform 300ms; on .card and transition: background-color 300ms, color 300ms; on .card__button) and rename .card__name to .card__title to align with the specification. Overall, you’ve done solid work on BEM, colors, basic layout, and hover effects—once you address the grid breakpoints and stars logic, your solution will be in great shape.


✨ 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
>
Video
</a>
</li>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similarly to the card, this button only sets transition-duration without specifying which properties to animate. Since both background-color and color change on :hover per the requirements, define transition: background-color 300ms, color 300ms; (or all 300ms) so the hover effect is guaranteed to be smooth.

Comment thread src/styles/card.scss
@@ -0,0 +1,108 @@
.card {
width: 200px;
height: 408px;

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 repeat(auto-fit, 200px), but checklist item #4 and the description require explicit column behavior: 1 column on small screens, then 2 / 3 / 4 columns at 488px / 768px / 1024px. You need media queries (or equivalent) to enforce those exact breakpoints instead of relying on auto-fit.

Comment thread src/styles/card.scss Outdated
max-width: 100%;
}

&__name {

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 description specifies changing the card title color via .card:hover .card__title, but here the element is named .card__name. Consider renaming this element to card__title and updating the hover selector so it matches the requirement exactly.

Comment thread src/styles/card.scss Outdated
color: $main-color;
}

&:hover &__name {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

&:hover &__name works, but the spec mentions .card:hover .card__title. After renaming the element, update this selector to match the required pattern (e.g. &:hover &__title).

Comment thread src/styles/card.scss Outdated
border: 1px solid #f3f3f3;
border-radius: 5px;
box-sizing: border-box;
transition-duration: 300ms;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

transition-duration: 300ms; satisfies the timing, but to make hover changes explicit and robust, consider specifying which properties should transition (e.g. transition: transform 300ms;) instead of relying on defaults.

Comment thread src/styles/nav.scss
@@ -0,0 +1,47 @@
.nav {
&__list {
display: flex;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

grid-template-columns: repeat(auto-fit, 200px); uses grid correctly, but it does not explicitly implement the requirement "Cards are shown in 1, 2, 3, or 4 columns based on screen resolution" with breakpoints at 488px, 768px, and 1024px. You should use media queries to set 1/2/3/4 columns at those exact widths instead of relying on auto-fit.

Comment thread src/styles/nav.scss
display: flex;
padding: 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.

The nested selector &--1 &:nth-child(-n + 1) compiles to something like .stars__star--1 .stars__star:nth-child(-n + 1), which assumes a parent with class .stars__star--1 wrapping .stars__star elements. In your HTML, the modifier is on the container (class="stars stars__star--4"), so this pattern never matches. This breaks the rating highlight logic; you should move the modifier to the container (e.g. .stars--4) and adjust the SCSS selectors accordingly.

Comment thread src/styles/nav.scss Outdated
line-height: 100%;
height: $header-height;
padding: 0;
transition: all 300ms;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similarly, &--4 &:nth-child(-n + 4) suffers from the same structural issue as the other modifiers: the compiled selector doesn’t match your actual HTML structure, so the correct stars won’t get the active background. Rework the modifiers so the container class and SCSS selectors line up.

Comment thread src/styles/nav.scss
padding: 0;
margin: 0;
gap: 20px;
list-style: none;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using only transition-duration: 300ms; on the card doesn’t clearly specify which properties should animate. Since you scale the card on hover, consider transition: transform 300ms; (or transition: all 300ms;) so the hover change is explicitly smooth as required.

Comment thread src/styles/stars.scss Outdated
Comment on lines +6 to +13
&__star {
background-image: url(../images/star.svg);
background-repeat: no-repeat;
background-position: center;
height: 16px;
width: 16px;

&--1 &:nth-child(-n + 1) {

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 stars modifiers are nested under .stars__star (&--1, &--2, etc.), which produces selectors like .stars__star--1 .stars__star:nth-child(-n + 1). But your HTML uses class="stars stars__star--4" on the parent container, not on .stars__star. Either move these modifiers to the .stars level (so you get .stars--4 .stars__star:nth-child(-n + 4)) or apply the modifier classes to the correct element in the markup so the rating highlighting works as intended.

@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 is still one critical, blocking issue with the stars component that affects the required rating behavior. In src/styles/stars.scss, the modifier selectors (e.g., .stars--4) currently target &:nth-child(-n + X) directly on .stars, which means they will apply the active star image to all children of .stars, not specifically to .stars__star elements; this violates the requirement that the stars block highlight only the star icons correctly [CHECKLIST ITEM #6]. To fix this, inside the .stars block, nest the modifier rules under &__star, for example:

.stars {
  &__star { /* base star styles */ }

  &--4 &__star:nth-child(-n + 4) {
    /* active star styles */
  }
}

This ensures that only .stars__star elements are affected when you use class="stars stars--4" in the HTML and that the rating highlighting works as intended. Aside from this selector issue, the rest of your layout, grid behavior, hover effects, BEM/SCSS structure, and required attributes look solid, so once you adjust the stars selectors, your solution should fully meet 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/stars.scss Outdated
Comment on lines +14 to +26
&--1 &:nth-child(-n + 1) {
background-image: url(../images/star-active.svg);
}

&--2 &:nth-child(-n + 2) {
background-image: url(../images/star-active.svg);
}

&--3 &:nth-child(-n + 3) {
background-image: url(../images/star-active.svg);
}

&--4 &:nth-child(-n + 4) {

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 #6 (and the stars functional requirement) because the rating modifiers target &:nth-child(-n + X) directly on the .stars block, which will include non-star children and doesn’t explicitly scope to .stars__star. Inside .stars, consider nesting the modifier rules under &__star (e.g., .stars--4 .stars__star:nth-child(-n + 4)) so only star elements receive the active background image and the rating highlighting works as intended.

@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 approving your solution as an educational override so you can keep progressing while you learn, even though there is still one checklist point to refine. You’ve done a strong job with the overall structure: semantic HTML with <header>, <nav>, and <main>, proper BEM blocks (header, nav, card, stars), responsive grid with 1/2/3/4 columns, and smooth hover transitions on cards, nav links, and buttons all match the task description well. The stars component is now correctly scoped so that modifiers apply to stars__star elements, which fixes the earlier blocking issue.

The main remaining improvement is about SCSS variables [CHECKLIST ITEM #2]: colors like #00acdc, #34568b, #616070, #f3f3f3, and #fff are still hard-coded in several files (for example, #00acdc in src/styles/nav.scss for hover and active states, and in src/styles/stars.scss for the accent color). The requirement is that repeated “design” colors be defined once as variables (e.g. in index.scss — something like $accent-color: #00acdc;) and then reused everywhere instead of repeating the literal. As a learning step, go through your SCSS, find each color that appears multiple times, create a clearly named variable, and replace those literals with the variable name. Overall, your layout, behavior, and BEM usage are in very good shape—tightening up the variable usage will make your code cleaner and closer to real-world professional CSS practices.


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

Footnotes

  1. Rate AI review example

Comment thread src/styles/nav.scss
transition: color 300ms;

&:hover {
color: #00acdc;

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 still violates checklist item #2: Repeated sizes and special colors are put to variables. Here, #00acdc is hard-coded for hover; since the same color is used multiple times (in .is-active and in the card button), it should be extracted into a variable (for example $accent-color) and referenced instead of the literal value.

Comment thread src/styles/nav.scss
Comment on lines +35 to +43
color: #00acdc;

&::after {
position: absolute;
display: block;
content: '';
height: 4px;
width: 100%;
background-color: #00acdc;

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 #2: Repeated sizes and special colors are put to variables. The color #00acdc is repeated here for text and background; define a shared SCSS variable and reuse it instead of duplicating the literal.

Comment thread src/styles/nav.scss
Comment on lines +14 to +30
&__link {
display: flex;
align-items: center;
text-decoration: none;
color: $main-color;
text-transform: uppercase;
font-size: 12px;
font-weight: 500;
line-height: 100%;
height: $header-height;
padding: 0;
transition: color 300ms;

&:hover {
color: #00acdc;
}
}

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 stars selector structure now correctly scopes modifiers to &__star, so the previous blocking issue is resolved; no further changes needed here.

Comment thread src/styles/stars.scss
Comment on lines +27 to +28
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 still violates checklist item #2: Repeated sizes and special colors are put to variables. Here #00acdc is used directly instead of via a shared SCSS variable (and it also appears elsewhere, like in .is-active). Define an accent-color variable in index.scss and use it here.

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