Skip to content

Catalogue#6287

Open
mirlik wants to merge 13 commits into
mate-academy:masterfrom
mirlik:develop
Open

Catalogue#6287
mirlik wants to merge 13 commits into
mate-academy:masterfrom
mirlik:develop

Conversation

@mirlik

@mirlik mirlik commented Jun 26, 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 several core requirements from the task description and checklist are still not implemented, so the solution is not yet functionally complete. The most critical blockers are: (1) the navigation state is wrong — the first Apple link must have class="nav__link is-active", but is-active is currently on the 4th link; and nav links must use color #060b35 (via $nav-link-color) instead of #000, with hover behavior targeting data-qa="nav-hover" and a transition: color 300ms; for smooth changes. (2) Old testing hooks were not removed: cards 2–8 still have data-qa="hover" on their Buy buttons, even though the spec requires removing old data-qa attributes and keeping only data-qa="nav-hover" on the 4th nav link, data-qa="card" on the first card, and data-qa="card-hover" on the first card’s Buy link. (3) Card hover behavior is missing: .card does not scale by 20% on hover (no transform: scale(1.2)), there is no transition on .card, and there is no rule like .card:hover .card__content-title { color: #34568b; }, so the required visual interaction is not present and [CHECKLIST ITEM #5] (“All changes on :hover are smooth”) is violated. (4) Hover transitions are incomplete in general: .nav__link and .card__button change color on hover but lack transition properties (e.g. transition: background-color 300ms, color 300ms;), so the hover effects are not smooth as required.

To fix this, please: (a) move is-active to the Apple link and set .nav__link base color to $nav-link-color, updating the hover selector to [data-qa='nav-hover'] with a 300ms color transition; (b) remove all data-qa="hover" from cards 2–8, leaving only the three specified testing attributes; (c) add transition and :hover styles on .card to scale it by 20% and change .card__content-title to #34568b on hover; and (d) add 300ms transitions to .card__button so its background/text color changes are smooth. Your grid layout, card sizing, and general BEM/SCSS structure are solid, so once you address these behavioral and data-attribute issues, the solution should fully match 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/index.html Outdated
Comment on lines +37 to +40
<li class="nav__item">
<a
href="#apple"
class="nav__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.

The first nav link (Apple) is missing the is-active class required by the description: "add the class is-active to the first link (Apple) in the navigation"; currently is-active is incorrectly applied to the 4th link instead.

Comment thread src/index.html Outdated
Comment on lines +186 to +189
<a
href="#"
class="card__button"
data-qa="hover"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Old data-qa attributes should be removed except for the specific ones mentioned in the task; this data-qa="hover" violates the requirement to "remove old data-qa attributes" and only add data-qa="card" and data-qa="card-hover" to the first card.

Comment thread src/index.html Outdated
Comment on lines +228 to +231
<a
href="#"
class="card__button"
data-qa="hover"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same as other cards: data-qa="hover" here is an old attribute that should be removed; only the first card’s Buy link should have data-qa="card-hover".

Comment thread src/index.html Outdated
Comment on lines +270 to +273
<a
href="#"
class="card__button"
data-qa="hover"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Again, data-qa="hover" on this Buy button conflicts with the requirement to remove old data-qa attributes and only keep the specific testing hooks for the first card.

Comment thread src/index.html Outdated
Comment on lines +312 to +315
<a
href="#"
class="card__button"
data-qa="hover"

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 data-qa="hover" attribute should be removed to comply with the requirement that old data-qa attributes be removed and replaced only with the specified data-qa="card" and data-qa="card-hover" on the first card.

Comment thread src/index.html Outdated
Comment on lines +354 to +357
<a
href="#"
class="card__button"
data-qa="hover"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same issue: this data-qa="hover" is not part of the required testing attributes and should be removed as per the description.

Comment thread src/index.html Outdated
Comment on lines +396 to +399
<a
href="#"
class="card__button"
data-qa="hover"

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 as well, data-qa="hover" is an obsolete attribute and should be removed; only the first card’s Buy button needs data-qa="card-hover".

Comment thread src/index.html Outdated
Comment on lines +438 to +441
<a
href="#"
class="card__button"
data-qa="hover"

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 last card still uses data-qa="hover", which contradicts the requirement to remove old data-qa attributes and keep only the explicitly requested ones.

Comment thread src/index.html Outdated
Comment on lines +32 to +36
class="logo"
/>
</a>
<nav class="nav">
<ul class="nav__list">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nav links should use the color #060b35 (via $nav-link-color) instead of #000, per the description and checklist item about using variables for special colors.

Comment thread src/index.html
Comment on lines +43 to +45
</a>
</li>
<li class="nav__item">

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 styles here do not use the required data-qa="nav-hover" selector (they still target [data-qa='hover']), and there is no transition defined for smooth color change during 300ms as required by checklist item #5.

Comment thread src/index.html
Comment on lines +76 to +98
</a>
</li>
<li class="nav__item">
<a
href="#tablets"
class="nav__link"
>
tablets
</a>
</li>
<li class="nav__item">
<a
href="#photo"
class="nav__link"
>
photo
</a>
</li>
<li class="nav__item">
<a
href="#video"
class="nav__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.

The button hover effect changes colors but no transition is defined, so changes won’t be smooth as required: "Make all the changes smooth on hover (during 300ms)" and checklist item #5.

Comment on lines +37 to +40
}

&__reviews {
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.

The first nav link (Apple) is missing the required is-active class, and the 4th link (laptops & computers) has is-active instead. This violates the description requirement: "add the class is-active to the first link (Apple) in the navigation."

Comment on lines +61 to +65

&__price-value {
font-size: 16px;
font-weight: 700;
line-height: 18px;

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 correctly add data-qa="nav-hover", but the spec requires it on the 4th nav link only and also that the first link be is-active, not this one. Move is-active to the first link, keep data-qa="nav-hover" here.

Comment thread src/styles/block/card.scss Outdated
Comment on lines +32 to +36

font-size: 10px;
font-weight: 400;
line-height: 14px;
color: $color-text-muted;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nav links are required to have color #060b35 (or use $nav-link-color from variables.scss), but here you still use color: #000;. This violates the description and checklist item #2 about using variables for repeated special colors.

Comment on lines +43 to +45
color: $color-text-main;
}

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 still targets [data-qa='hover'] on hover, but the updated requirement uses data-qa="nav-hover" on the 4th link and asks to remove old data-qa attributes. This rule should be updated or removed, and all hover styles should be made smooth with a transition (300ms), per checklist item #5.

Comment thread src/styles/block/card.scss Outdated
Comment on lines +32 to +41

font-size: 10px;
font-weight: 400;
line-height: 14px;
color: $color-text-muted;
}

&__reviews {
font-size: 10px;
font-weight: 400;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You change link color on hover/active but there is no transition on .nav__link, so the color change is instant rather than smooth. This violates checklist item #5: "All changes on :hover are smooth." Consider adding a transition of 300ms for color (and any other hover-affected properties).

Comment on lines +1 to +100
.card {
display: flex;
flex-direction: column;

box-sizing: border-box;
width: $card-width;
padding: 32px 17px 17px;
border-radius: 5px;
border: solid 1px #f3f3f3;
font-family: $font-main;

&__img {
display: block;
width: 160px;
height: 134px;
margin: auto;
}

&__content-title {
margin: 0;
margin-top: 40px;

font-size: 12px;
font-weight: 500;
line-height: 18px;
color: $color-text-main;
}

&__content-code {
margin: 0;
margin-top: 4px;

font-size: 10px;
font-weight: 400;
line-height: 14px;
color: $color-text-muted;
}

&__reviews {
font-size: 10px;
font-weight: 400;
line-height: 14px;
color: $color-text-main;
}

&__footer {
display: flex;
align-items: center;
justify-content: space-between;

margin: 0;
margin-top: 24px;
}

&__price-label {
font-size: 12px;
font-weight: 400;
line-height: 18px;
color: $color-text-muted;
}

&__price-value {
font-size: 16px;
font-weight: 700;
line-height: 18px;
color: $color-text-main;
}

&__rating-row {
display: flex;
align-items: center;
justify-content: space-between;
margin-top: 16px;
}

&__button {
display: flex;
align-items: center;
justify-content: center;

box-sizing: border-box;
height: 40px;
margin-top: 16px;
border: 1px solid $color-primary;
border-radius: 5px;

font-size: 14px;
font-weight: 700;
line-height: 14px;
color: white;
text-decoration: none;

background-color: $color-primary;

&:hover {
color: $color-primary;
background-color: white;
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Card hover behavior is missing: the description requires increasing the card by 20% on hover and changing the title color on .card:hover, with smooth 300ms transitions. Currently there is no transition or :hover on .card, which violates checklist item #5 and the hover behavior requirements.

Comment on lines +76 to +98
&__button {
display: flex;
align-items: center;
justify-content: center;

box-sizing: border-box;
height: 40px;
margin-top: 16px;
border: 1px solid $color-primary;
border-radius: 5px;

font-size: 14px;
font-weight: 700;
line-height: 14px;
color: white;
text-decoration: none;

background-color: $color-primary;

&:hover {
color: $color-primary;
background-color: white;
}

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 button correctly inverts colors on hover, but there is no transition defined, so the change is not smooth as required ("Make all the changes smooth on hover (during 300ms)"). Add a transition (e.g. for background-color and color) to satisfy checklist item #5.

Comment on lines +1 to +7
.card {
display: flex;
flex-direction: column;

box-sizing: border-box;
width: $card-width;
padding: 32px 17px 17px;

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 hard-codes 200px instead of reusing the $card-width variable. Since repeated sizes and special colors should be put into variables (checklist item #2), consider connecting this value to the SCSS variable to avoid duplication.

@@ -0,0 +1,23 @@
.catalog {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Card hover behavior is missing entirely. The description requires: 1) increasing the card size by 20% on hover (without affecting neighbors), 2) changing the card title color on hover, and 3) making these changes smooth over 300ms. There is no transition or :hover rule on .card, violating checklist item #5: “All changes on :hover are smooth,” and the hover behavior requirements.


grid-template-columns: repeat(var(--column-count), 200px);

gap: 46px 48px;

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 border color #f3f3f3 is a special color that’s not in variables.scss. This violates checklist item #2: “Repeated sizes and special colors are put to variables.” Consider extracting it to a SCSS variable and reusing it.

padding: 50px 40px;
display: grid;

grid-template-columns: repeat(var(--column-count), 200px);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

catalog uses CSS Grid and correct column counts at breakpoints, which is good, but note that the card hover effect (scale + smooth transition) is still missing in card.scss, so the visual behavior required for the catalog is incomplete.

Comment thread src/styles/block/header.scss Outdated
@@ -0,0 +1,58 @@
:root {

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 card block needs hover behavior and smooth transitions as per the description (scale card by 20%, change title color to #34568b, 300ms transitions). Currently there is no transition or card :hover here, which violates checklist item #5: “All changes on :hover are smooth.” Consider adding transition and &:hover styles on the card and title element.

Comment thread src/styles/block/header.scss Outdated
display: flex;
align-items: center;
height: 60px;
color: #000;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nav links are required to have base color #060b35, but here they use color: #000;. Also you introduced $nav-link-color in variables.scss but don’t use it, which violates checklist item #2: “Repeated sizes and special colors are put to variables.” Replace #000 with $nav-link-color (through a variable or a shared class).

Comment thread src/styles/block/header.scss Outdated
Comment on lines +43 to +45
&__link:hover,
&__link.is-active,
&__link[data-qa='hover']:hover {

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 still targets data-qa='hover', but the description says old data-qa attributes must be removed and replaced with data-qa="nav-hover" on the 4th nav link. Adjust this selector to use [data-qa='nav-hover'] and also add a transition: color 300ms; on the link so hover color changes are smooth per checklist item #5.

Comment thread src/styles/block/header.scss Outdated
Comment on lines +1 to +2
:root {
--blue: #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.

You’re using a CSS variable --blue for the hover color, while the project already defines $color-primary and $nav-link-color in SCSS. To satisfy checklist item #2 about variables and keep consistency, move this color to SCSS variables (or reuse existing ones) instead of mixing CSS custom properties and SCSS vars.

Comment thread src/styles/index.scss
Comment on lines +1 to 7
@import 'variables';
@import 'block/header';
@import 'block/stars';
@import 'block/card';
@import 'block/catalog';

body {

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 hard-codes 200px instead of reusing $card-width. Checklist item #2 says repeated sizes should be put into variables; consider referencing the SCSS variable here to avoid duplicating this magic number.

Comment thread src/styles/index.scss Outdated
Comment on lines +5 to +9
@import 'block/catalog';

body {
margin: 0;
font-weight: 400;

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 border color #f3f3f3 is a special color not defined in variables.scss. Checklist item #2 requires special colors to be stored in variables; extract this into a SCSS variable for consistency.

Comment thread src/styles/variables.scss
@@ -0,0 +1,6 @@
$font-main: 'Roboto', sans-serif;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cards must grow by 20% on hover and all hover changes must be smooth over 300ms, but there is no transition or :hover on .card at all. Implement a transition and a :hover state (e.g. with transform: scale(1.2)) and change the title color on hover to satisfy the hover behavior and checklist item #5.

Comment thread src/styles/variables.scss Outdated
Comment on lines +1 to +6
$font-main: 'Roboto', sans-serif;
$color-primary: rgba(0, 172, 220, 1);
$color-text-main: rgba(6, 11, 53, 1);
$color-text-muted: rgba(97, 96, 112, 1);
$card-width: 200px;
$nav-link-color: #060b35;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You defined $nav-link-color here but are not using it in header.scss for .nav__link, and nav links still use #000. This partially violates checklist item #2 (colors in variables) and the requirement that nav links be #060b35. Replace hard-coded #000 with $nav-link-color.

@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 one core interaction from the task description is still missing and one required hover behavior is not smooth yet, so the implementation does not fully meet the functional/checklist requirements.

Critical issues that must be fixed (blockers):

  1. The card title color does not change when the card is hovered. In card.scss inside &__content-title you currently have a nested selector like &:hover &____content-title, which compiles to an invalid selector and also ties the color change to hovering the title, not .card. The requirement says the title text must change to #34568b when the card is hovered (.card:hover .card__title). Update your SCSS so that hovering .card changes .card__content-title to $color-text-main-hovered (e.g. using .card:hover & inside &__content-title or an explicit .card:hover .card__content-title { color: $color-text-main-hovered; }) while keeping the existing 0.3s transition so the change is smooth [CHECKLIST ITEM #5].
  2. Nav link hover is not smooth yet. In src/styles/block/nav.scss, .nav__link has no transition on color, so the change to $nav-hover-color on :hover happens instantly. The corrections and checklist require all hover changes to be smooth, so add something like transition: color 300ms; to .nav__link so its color animation matches the behavior already implemented on .card and .card__button [CHECKLIST ITEM #5].

Minor but recommended improvement:

  • In header.scss, you hard-code font-family: Roboto, sans-serif; even though $font-main: 'Roboto', sans-serif; is defined in variables.scss and already used on .card. For consistency with your design tokens and to fully align with [CHECKLIST ITEM #2], switch the header to font-family: $font-main; instead of duplicating the font stack.

Overall structure, grid layout, data-qa attributes, card scaling, and button hover behavior look very good; once you fix the card-title hover selector and add the nav color transition, your solution 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 thread src/index.html
Comment on lines +24 to +33
<!-- #endregion -->
<body>
<h1>Catalog</h1>
<!-- #region header -->
<header class="header">
<a
href="#top"
class="header__logo"
>
<img
src="./images/logo.png"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inside &__content-title you have &:hover &____content-title, which does not match any real element and also makes the color change depend on hovering the title itself instead of the whole card. This violates the requirement to change the card title text color via .card:hover .card__title when the card is hovered (see description) and [CHECKLIST ITEM #5] about smooth hover changes. Update your SCSS so that hovering .card changes the title color, for example using a selector like .card:hover & inside &__content-title (or an equivalent .card:hover .card__content-title rule) and ensure the color transition remains smooth.

Comment thread src/styles/block/card.scss Outdated
Comment on lines +24 to +33
&__content-title {
margin: 0;
margin-top: 40px;

font-size: 12px;
font-weight: 500;
line-height: 18px;
color: $color-text-main;

&:hover &____content-title {

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 selector inside &__content-title (&:hover &____content-title) is incorrect and does not implement the required behavior from the description: the title color must change to #34568b when the card is hovered (.card:hover .card__title). You should move this rule to the .card hover context and target .card__content-title, using $color-text-main-hovered so the change is smooth with your existing transition. This violates checklist item #5: All changes on :hover are smooth for the card title.


box-sizing: border-box;
width: $card-width;
padding: 32px 17px 17px;

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 hard-code font-family: Roboto, sans-serif; even though $font-main: 'Roboto', sans-serif; is defined in variables.scss and already used on .card. To satisfy checklist item #2 Repeated sizes and special colors are put to variables, use the $font-main variable here instead of duplicating the value.

Comment thread src/styles/block/nav.scss
Comment on lines +24 to +33
&__link:hover {
color: $nav-hover-color;
}

&__link.is-active::after {
content: '';
bottom: 0;
left: 0;
position: absolute;
width: 100%;

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 nested rule &:hover &____content-title does not implement the required behavior. It targets a non-existent selector and depends on hovering the title itself instead of the card. The description requires the title color to change when the card is hovered (.card:hover .card__title), so you should instead target .card:hover .card__content-title (e.g. using .card:hover & inside this block) and set color: $color-text-main-hovered;. This currently violates the hover requirement and checklist item #5.

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

Here you hardcode font-family: Roboto, sans-serif; even though $font-main: 'Roboto', sans-serif; is defined in variables.scss and already used on .card. To align with checklist item #2 (Repeated sizes and special colors are put to variables), use $font-main in the header instead of duplicating the font value.

&__star {
height: 16px;
width: 16px;
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.

Here you hard-code font-family: Roboto, sans-serif; even though $font-main: 'Roboto', sans-serif; is defined in variables.scss and already used on .card. To fully satisfy checklist item #2 (“Repeated sizes and special colors are put to variables”), use $font-main here instead of duplicating the font stack.

@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 practice these patterns. You did a solid job with the semantic HTML structure, the grid-based catalog layout, responsive column counts, BEM naming, and most hover behaviors, which shows good attention to the main requirements. There are still a few important things to refine next time: in src/styles/block/card.scss, the selector inside &__content-title currently compiles incorrectly (.card__content-title:hover .card__content-title) and should instead target the title when the card is hovered, equivalent to .card:hover .card__content-title { color: $color-text-main-hovered; } so the color change is smooth [CHECKLIST ITEM #5]. In src/styles/block/nav.scss, move the transition from &__link:hover onto the base .nav__link (e.g. transition: color 300ms;) so the nav link text color animates both on hover-in and hover-out, again for smooth hover behavior [CHECKLIST ITEM #5]. Also, in header.scss, catalog.scss, nav.scss, stars.scss, index.scss, and variables.scss, replace hard-coded font-family: Roboto, sans-serif; with the shared $font-main variable to fully follow the “use variables for repeated values” rule [CHECKLIST ITEM #2]. Overall, your structure and styling fundamentals are strong; tightening these small details will make your future implementations even closer to production-quality code.


✨ 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/block/card.scss Outdated
line-height: 18px;
color: $color-text-main;

&:hover &__content-title {

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 &:hover &__content-title compiles to .card__content-title:hover .card__content-title, which is invalid for the required behavior and does not depend on hovering .card. This violates the requirement: "change the card title text color to #34568b when the card is hovered (.card:hover .card__title)" and Checklist item #5 about smooth hover changes. Inside &__content-title, use a pattern that effectively yields .card:hover .card__content-title { color: $color-text-main-hovered; } so that hovering the whole card changes the title color with the existing transition.


box-sizing: border-box;
width: $card-width;
padding: 32px 17px 17px;

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 hard-code font-family: Roboto, sans-serif; even though $font-main: 'Roboto', sans-serif; is defined and used elsewhere. This conflicts with Checklist item #2: "Repeated sizes and special colors are put to variables". Use font-family: $font-main; so the header reuses the shared font variable.

Comment on lines +1 to +7
.catalog {
--column-count: 1;

padding: 50px 40px;
display: grid;

grid-template-columns: repeat(var(--column-count), 200px);

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 still hard-code font-family: Roboto, sans-serif; even though $font-main: 'Roboto', sans-serif; exists in variables.scss. This goes against checklist item #2: "Repeated sizes and special colors are put to variables" and the previous review note to replace this with font-family: $font-main;.

Comment thread src/styles/block/header.scss Outdated
justify-content: space-between;
padding-inline: 50px;

font-family: Roboto, sans-serif;

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 a hard-coded font-family: Roboto, sans-serif; here duplicates the value already stored in $font-main. This violates checklist item "Repeated sizes and special colors are put to variables" and the previous review note about header.scss. Replace this with font-family: $font-main;.

Comment thread src/styles/block/nav.scss Outdated
Comment on lines +24 to +33
&__link:hover {
transition: 0.3s;
color: $nav-hover-color;
}

&__link.is-active::after {
content: '';
bottom: 0;
left: 0;
position: absolute;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inside &__content-title this &:hover &__content-title compiles to something like .card__content-title:hover .card__content-title, which is not what the task requires and is effectively invalid for the intended behavior. The description says: "change the card title text color to #34568b when the card is hovered (.card:hover .card__title)", and checklist item #5 requires this hover change to be smooth. You should instead target the title when the card is hovered (e.g. using .card:hover & inside &__content-title or a separate .card:hover .card__content-title { color: $color-text-main-hovered; }).

Comment thread src/styles/block/nav.scss Outdated
Comment on lines +24 to +25
&__link:hover {
transition: 0.3s;

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 the nav link’s transition is defined only on &__link:hover. For the hover color change to be smooth as required by checklist item "All changes on :hover are smooth", the transition should be on the base .nav__link rule (e.g. transition: color 300ms;), while &__link:hover should only change the color. Defined this way, color will animate both on hover-in and hover-out.

Comment thread src/styles/block/nav.scss
padding: 0;
margin: 0;
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.

This hard-coded font-family: Roboto, sans-serif; duplicates the value already defined as $font-main in variables.scss. Checklist item "Repeated sizes and special colors are put to variables" plus the previous correction require using the variable instead, so replace this with font-family: $font-main;.

Comment on lines +1 to +7
.stars {
display: flex;

&__star {
height: 16px;
width: 16px;
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.

Here you hard-code font-family: Roboto, sans-serif; even though $font-main: 'Roboto', sans-serif; is defined in variables.scss. This goes against Checklist item #2: "Repeated sizes and special colors are put to variables". Replace this with font-family: $font-main; so the header uses the shared font variable.

Comment thread src/styles/index.scss
@import 'block/card';
@import 'block/stars';
@import 'block/catalog';

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 still hard-code font-family: Roboto, sans-serif; even though $font-main is defined in variables.scss and already used on .card. This goes against Checklist item #2: “Repeated sizes and special colors are put to variables.” Replace this with font-family: $font-main; so the header uses the shared font variable.

Comment thread src/styles/variables.scss
$color-primary: #00acdc;
$color-text-main: #060bff;
$color-text-main-hovered: #34568b;
$color-text-muted: #616070;

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 still hard-code font-family: Roboto, sans-serif; even though $font-main: 'Roboto', sans-serif; is defined in variables.scss. This conflicts with checklist item #2: "Repeated sizes and special colors are put to variables." Replace this with font-family: $font-main; to reuse the variable.

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

good job, but text should be a different color

Image

@mirlik

mirlik commented Jun 26, 2026

Copy link
Copy Markdown
Author

I hope this commit was the last one on this exercise )

@mirlik mirlik requested a review from etojeDenys June 26, 2026 20:16

@Anton-Kuchmasov Anton-Kuchmasov 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.

Good job, but don't forget to run npm t before deploy each single time when you're making changes in your code

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.

4 participants