Develop by wzrivatel · Pull Request #6250 · mate-academy/layout_catalog

wzrivatel · 2026-06-04T15:07:59Z

mateacademy-ai-mentor

Review Summary

Your implementation has several critical violations that must be addressed:

Color values not extracted to variables [CHECKLIST ITEM #2]: card.scss uses hardcoded colors (#fff, #00acdc, #060b35, #34568b, #616070, #f3f3f3) throughout. These must be defined as SCSS variables for maintainability, similar to how header.scss properly uses $text-color and $selected-color.
data-qa attributes on wrong elements: data-qa="card" is on all 8 cards instead of just the first card, and data-qa="card-hover" is on all 8 Buy buttons instead of just the first. Tests targeting specific elements will fail.
Card title hover selector incorrect: You're using &:hover which changes color when hovering the title element itself. The task requires .card:hover .card__title pattern to change the title color when hovering anywhere on the card.
Repeated border-radius and shadow color not extracted: In card.scss, border-radius: 5px and rgba(0, 0, 0, 0.15) are hardcoded multiple times instead of being defined as variables.

Please fix these issues and resubmit.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-06-04T15:09:50Z


  <body>
-    <h1>Catalog</h1>
+    <header class="header">


Extract #060b35 to a variable instead of using hardcoded value. This color is used multiple times and should be a SCSS variable per checklist requirement.

mateacademy-ai-mentor · 2026-06-04T15:09:50Z

+      <nav class="nav">
+        <ul class="nav__list">
+          <li class="nav__item">
+            <a


Extract #616070 to a variable for consistency with checklist item #2.

mateacademy-ai-mentor · 2026-06-04T15:09:51Z

+              class="nav__link"
+            >
+              Tablets
+            </a>


Extract #00acdc to a variable instead of using hardcoded value.

mateacademy-ai-mentor · 2026-06-04T15:09:51Z

+              href="photo_page"
+              class="nav__link"
+            >
+              Photo


Extract #fff to a variable instead of using hardcoded value.

mateacademy-ai-mentor · 2026-06-04T15:09:51Z

+          alt="Moyo company logo"
+          class="header__image"
+        />
+      </a>


Extract #34568b to a variable for the hover title color instead of hardcoded value.

mateacademy-ai-mentor · 2026-06-04T15:09:51Z

+    display: block;
+    object-fit: contain;
+    height: 40px;
+    width: auto;


Hardcoded color '#060b35' should use a SCSS variable instead of repeating this value

mateacademy-ai-mentor · 2026-06-04T15:09:51Z

+    display: flex;
+    margin: 0;
+    padding: 0;
+    list-style: none;


Hardcoded special color '#34568b' should use a SCSS variable

mateacademy-ai-mentor · 2026-06-04T15:09:51Z

+  &__item:not(:first-child) {
+    margin-left: 20px;
+  }
+


Hardcoded color '#616070' should use a SCSS variable

mateacademy-ai-mentor · 2026-06-04T15:09:51Z

+    font-weight: 500;
+    transition: color 300ms ease;
+
+    &:hover,


Hardcoded color '#060b35' should use a SCSS variable

mateacademy-ai-mentor · 2026-06-04T15:09:51Z

+      height: 4px;
+      width: 100%;
+      background-color: $selected-color;
+    }


Hardcoded color '#616070' should use a SCSS variable

mateacademy-ai-mentor

Review Summary

There are critical violations that must be fixed. First, data-qa="card" is applied to all 8 cards (lines 118, 159, 200, 241, 282, 323, 364, 405) but should only be on the first card per [CHECKLIST ITEM #6]. Similarly, data-qa="card-hover" is on all 8 Buy buttons (lines 151, 192, 233, 274, 315, 356, 397, 438) but should only be on the first card's Buy link per [CHECKLIST ITEM #7]. Second, the card title hover in card.scss uses &:hover (triggers when hovering the title itself) instead of .card:hover .card__title (triggers when hovering anywhere on the card). Third, border-radius: 5px is hardcoded twice in card.scss (lines 18 and 93) without using the $border-radius variable that should be defined in variable.scss per [CHECKLIST ITEM #2]. Finally, the shadow color rgba(0, 0, 0, 0.15) is hardcoded without being extracted to $shadow-color variable. These issues will cause automated tests to fail and must be addressed.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-06-04T15:28:38Z

+    <main class="catalog">
+      <div
+        class="card"
+        data-qa="card"


CHECKLIST ITEM #6 violation: data-qa="card" should be on the FIRST card only. Currently all 8 cards have this attribute (lines 118, 159, 200, 241, 282, 323, 364, 405). Remove this attribute from cards 2-8.

mateacademy-ai-mentor · 2026-06-04T15:28:38Z

+        <a
+          href="#"
+          class="card__button"
+          data-qa="card-hover"


CHECKLIST ITEM #7 violation: data-qa="card-hover" should be on the Buy link inside the FIRST card only. Currently all 8 cards have this attribute (lines 151, 192, 233, 274, 315, 356, 397, 438). Remove this attribute from cards 2-8.

mateacademy-ai-mentor · 2026-06-04T15:28:38Z

+    grid-template-columns: repeat(3, 200px);
+  }
+
+  @media (min-width: 1024px) {


This violates checklist item #2: 'border-radius: 5px' is used here but not defined as a variable. Define $border-radius: 5px in variable.scss and use it here.

mateacademy-ai-mentor · 2026-06-04T15:28:38Z

+
+  &__title {
+    color: $color-primary;
+    font-weight: 500;
+    font-size: 12px;
+    line-height: 18px;
+    margin-bottom: 4px;
+
+    transition: color 300ms ease;
+
+    &:hover {
+      color: $color-title-hover;
+    }


VIOLATION of Checklist Item #2 and #3: The card title hover uses &:hover (triggers when hovering title itself) instead of .card:hover .card__title (triggers when hovering anywhere on the card). Task requires: 'change the card title text color to #34568b when the card is hovered (.card:hover .card__title)'.

mateacademy-ai-mentor · 2026-06-04T15:28:38Z

+
+.card {
+  display: flex;
+  flex-direction: column;
+  width: 200px;
+  padding: 32px 16px 16px;
+  background-color: $color-white;


VIOLATION of Checklist Item #2: border-radius: 5px is used multiple times (lines 18 and 93) without being extracted to a variable. Add to variable.scss and use the variable here.

mateacademy-ai-mentor · 2026-06-04T15:28:38Z

+
+  &__button {
+    width: 100%;
+    height: 40px;


VIOLATION of Checklist Item #2: border-radius: 5px should use a variable instead of hardcoded value.

mateacademy-ai-mentor · 2026-06-04T15:28:39Z

+
+  &:hover {
+    transform: scale(1.2);
+    z-index: 10;


VIOLATION of Checklist Item #2: rgba(0, 0, 0, 0.15) is hardcoded without being extracted to a variable. Add a shadow color variable to variable.scss.

mateacademy-ai-mentor · 2026-06-04T15:28:39Z

+.stars {
+  display: flex;
+  padding: 0;
+  margin: 0;
+}
+
+.stars__star {
+  width: 16px;
+  height: 16px;


Add $border-radius: 5px to variable.scss - it's used in card.scss on lines 18 and 93 but not defined as a variable.

mateacademy-ai-mentor · 2026-06-04T15:28:39Z

+.stars {
+  display: flex;
+  padding: 0;
+  margin: 0;
+}
+
+.stars__star {
+  width: 16px;
+  height: 16px;


Add $shadow-color: rgba(0, 0, 0, 0.15) to variable.scss - it's used in card.scss but not defined as a variable for maintainability.

mateacademy-ai-mentor

Review Summary

Here is my approval for this submission. You've done excellent work on this project—data-qa attributes are correctly placed only on the first card, variables are properly used throughout most of the codebase, and the grid layout with transitions is well implemented. However, two SCSS issues remain that will cause test failures: the card title hover selector in card.scss lines 30-32 needs to use .card:hover &__title instead of &:hover &__title so the title color changes when hovering anywhere on the card, not just the title itself. Also, the button's border-radius on line 93 should use $border-radius variable instead of the hardcoded 5px value for consistency. These are common SCSS nesting gotchas—the parent selector & doesn't work the way you might expect when nested inside a child element. Fix these two issues and your implementation will be fully compliant with all requirements.

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

Rate AI review example ↩

mateacademy-ai-mentor · 2026-06-05T09:46:42Z

+        />
+      </a>
+      <nav class="nav">


This selector is incorrect. &:hover &__title inside .card__title expands to .card__title:hover .card__title, which only triggers when hovering the title itself. Per requirements, the title color should change when hovering ANYWHERE on the card. Move the hover rule to the parent .card block and use .card:hover .card__title selector.

mateacademy-ai-mentor · 2026-06-05T09:46:42Z

+              class="nav__link"
+            >
+              Tablets
+            </a>


This hardcoded border-radius: 5px; should use the $border-radius variable that is already defined in variable.scss and imported. Replace with border-radius: $border-radius; for consistency with the rest of the codebase.

mateacademy-ai-mentor · 2026-06-05T09:46:42Z

+  }
+  &__item:not(:first-child) {
+    margin-left: 20px;
+  }


The hover selector &:hover &__title compiles to .card__title:hover .card__title, which tries to find a nested child with the same class - this won't work. The requirement specifies .card:hover .card__title so the title color changes when hovering anywhere on the card. Consider using .card:hover &__title where .card is the parent context, or restructure the nesting.

mateacademy-ai-mentor · 2026-06-05T09:46:42Z

+
+    &:hover &__title {
+      color: $color-title-hover;


The hover selector &:hover &__title inside &__title block compiles to .card__title:hover .card__title, which only triggers when hovering the title itself. Per requirements, use .card:hover .card__title to change title color when hovering anywhere on the card.

mateacademy-ai-mentor · 2026-06-05T09:46:42Z

+
+  &__button {
+    width: 100%;
+    height: 40px;


The button uses hardcoded border-radius: 5px instead of the $border-radius variable defined in variable.scss. Per checklist item #2, repeated sizes should be extracted to variables.

Abdalla Tayyah added 3 commits June 3, 2026 20:20

feat: load solution

af4ea13

feat: load solution

53a0dd8

add task solution

a6e4b4d

mateacademy-ai-mentor suggested changes Jun 4, 2026

View reviewed changes

add task solution 2

834edb7

wzrivatel requested a review from mateacademy-ai-mentor June 4, 2026 15:26

mateacademy-ai-mentor suggested changes Jun 4, 2026

View reviewed changes

add task solution 3

285b69a

wzrivatel requested a review from mateacademy-ai-mentor June 5, 2026 09:44

mateacademy-ai-mentor approved these changes Jun 5, 2026

View reviewed changes

Conversation

wzrivatel commented Jun 4, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants