add task solution#6243
Conversation
yulianaqq
commented
May 31, 2026
yulianaqq
commented
- DEMO LINK
- TEST REPORT LINK
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has critical issues that violate task requirements and cause functional problems. First, the card hover scaling applies transform: scale(1.2) to .card__content without overflow: hidden on .card, causing the 240px scaled content to overflow the 200px card boundary and overlap with neighboring cards—this directly violates the requirement that neighboring cards should not be affected. Second, the button hover background uses #f3f3f3 from $background-color-hover instead of the required #fff white background. Third, the card title hover targets .card:hover .card__link instead of the specified .card:hover .card__title selector. Additionally, the .logo class in header.scss/page.scss violates BEM methodology by being defined outside the .header block—change it to .header__logo. To fix the overflow issue, add overflow: hidden to .card and change the scale transform to use .card:hover .card__content { transform: scale(1.2); }. Update $background-color-hover to #fff for the button hover state. These are clear functional requirements that must be addressed before approval.
✨ 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
|
|
||
| &__content:hover { | ||
| transform: scale(1.2); |
There was a problem hiding this comment.
The transform: scale(1.2) is applied to .card__content, but .card has no overflow: hidden. This causes the scaled content (240px) to overflow the 200px card and overlap with neighboring cards, violating the requirement that neighboring cards should not be affected.
|
|
||
| .card:hover .card__link { | ||
| color: $color-link-card-hover; |
There was a problem hiding this comment.
The task requires the card title text color to change on card hover using .card:hover .card__title selector. Currently using .card:hover .card__link which targets the nested link instead of the title element directly.
| &:hover { | ||
| background-color: $background-color-hover; | ||
| border-color: $color-accent; | ||
| color: $color-accent; |
There was a problem hiding this comment.
The button hover background should be #fff according to requirements, but $background-color-hover is #f3f3f3.
| &__content { | ||
| height: 100%; | ||
| transition: transform 300ms; | ||
| } | ||
|
|
There was a problem hiding this comment.
The .logo class doesn't follow BEM convention. It should be .header__logo to properly belong to the header block.
| .header { | ||
| box-sizing: border-box; | ||
| width: 100%; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| padding: 0 50px; | ||
| } | ||
|
|
There was a problem hiding this comment.
The logo class violates BEM methodology. In BEM, elements should be named as block__element. Since logo is part of the header block, it should be defined as .header__logo inside the .header block, not as a standalone .logo class. The HTML also uses class="logo" which should be changed to class="header__logo".
| <li class="nav__item"> | ||
| <a |
There was a problem hiding this comment.
The .is-active::after selector is missing the parent reference &. Currently it targets .nav .is-active::after which searches for a .is-active descendant of .nav. The correct selector should be &__link.is-active::after or &.is-active::after to target the pseudo-element on the nav link itself.
| class="card__photo" | ||
| src="./images/imac.jpeg" | ||
| alt="iMac" | ||
| /> |
There was a problem hiding this comment.
The button hover background uses $background-color-hover which equals #f3f3f3. According to the requirements, the button background on hover should be #fff (white), not the light gray.
| $color-accent: #00acdc; | ||
| $color-main-button: #fff; | ||
| $border-color: #f3f3f3; | ||
| $background-color-hover: #f3f3f3; |
There was a problem hiding this comment.
The variable $background-color-hover is set to #f3f3f3, but the button hover effect requires a white background (#fff). This variable is used on line 114 of card.scss for the button hover state, which contradicts the requirement that states 'change the button background to #fff'.
| padding: 50px 40px; | ||
| grid-template-columns: repeat(1, 200px); | ||
| gap: 46px 48px; | ||
| justify-content: center; | ||
| } |
There was a problem hiding this comment.
The .logo class is defined outside the .header block (lines 9-13), which doesn't follow strict BEM methodology. In BEM, logo is an element of the header block and should be defined as .header__logo nested within the .header block. The class should also be renamed in HTML from class="logo" to class="header__logo" to maintain consistency.
| } | ||
|
|
||
| @media (min-width: 488px) { |
There was a problem hiding this comment.
The card scale transform is applied to .card__content:hover (self-hover) rather than .card:hover .card__content. This means the card only scales when directly hovering the content area. According to the requirement, the entire card should scale on hover. Consider using .card:hover .card__content { transform: scale(1.2); } to ensure the full card triggers the effect.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is missing a required feature: the card title hover color change. The task description explicitly requires adding .card:hover .card__title { color: $color-link-card-hover; } to card.scss to change the title text color to #34568b when hovering over a card. The $color-link-card-hover variable is already defined in variables.scss but is currently unused. The current implementation applies color to .card__link instead of targeting .card:hover .card__title as specified. Please add this hover rule to card.scss. Note: the visual test failures (1-2% difference) are likely occurring because this hover effect is missing from the implementation.
✨ 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
| line-height: 60px; | ||
| text-transform: uppercase; | ||
| text-decoration: none; | ||
| font-weight: 500; | ||
| font-size: 12px; | ||
| color: $color-main; |
There was a problem hiding this comment.
The description requires the selector .card:hover .card__title to change the title text color to #34568b when the card is hovered. Add this rule to card.scss: .card:hover .card__title { color: $color-link-card-hover; } using the existing $color-link-card-hover variable.
| <img | ||
| class="header__logo" | ||
| src="./images/logo.png" | ||
| alt="Moyo logo" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
The requirement specifies .card:hover .card__title should change the title text color to #34568b, but there's no color property on .card__title to change on hover. The title's color comes from .card__link which has color: $color-main. Add color: #34568b; to .card:hover .card__title to implement this hover effect.
|
|
||
| &__title { | ||
| font-size: 12px; | ||
| font-weight: 500; | ||
| line-height: 18px; | ||
| margin: 0 16px 4px; |
There was a problem hiding this comment.
The card title hover uses .card__link instead of .card:hover .card__title as explicitly specified in the task requirements. Add a hover rule targeting .card__title directly, or add .card:hover .card__title { color: $color-link-card-hover; } at the card level to match the exact selector requirement.
|
|
||
| @media (min-width: 1024px) { | ||
| .catalog { | ||
| grid-template-columns: repeat(4, 200px); | ||
| } | ||
| } |
There was a problem hiding this comment.
The requirement specifies using .card:hover .card__title selector to change the card title text color to #34568b. Add this rule to card.scss. The variable $color-link-card-hover is defined but unused. Add .card:hover .card__title { color: $color-link-card-hover; } to implement the missing hover effect.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution with an educational override to help you progress while learning. Your implementation demonstrates strong foundational skills—you've correctly applied BEM methodology across all components, extracted colors to variables, implemented responsive grid breakpoints (488px, 768px, 1024px), and placed all required data-qa attributes properly. The nav hover color and button transitions are working as expected. In src/styles/card.scss, there's a CSS specificity conflict: your hover rule targets .card__title but the text color is set on .card__link, so the hover color won't apply. Change the selector to .card:hover .card__link to fix this. Additionally, nest your hover rule inside the .card block using &:hover for proper BEM organization, and add a 300ms transition on the title color for smooth effect. When an element has a nested child (like &__link inside .card), the hover rule must target that child directly if it has an explicit color property, or remove the explicit color and let it inherit instead. Keep up the good work on your structure and attention to detail—these are exactly the skills that matter in real projects!
✨ 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
|
|
||
| .card:hover .card__title { | ||
| color: $color-link-card-hover; |
There was a problem hiding this comment.
The hover rule is outside the .card block which violates BEM organization. Should be nested using &:hover within the block.
|
|
||
| &__link { | ||
| display: block; | ||
| text-decoration: none; | ||
| color: $color-main; |
There was a problem hiding this comment.
The hover selector targets .card__title but the actual text color is set on .card__link. Since .card__link has an explicit color: $color-main, it won't inherit the hover color. Consider targeting .card:hover &__link or removing the explicit color from &__link so it inherits.
|
|
||
| .card:hover .card__title { | ||
| color: $color-link-card-hover; |
There was a problem hiding this comment.
The title color change on hover has no transition. Add transition: color 300ms for smooth effect as required.
3 participants
