created a grid#6295
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several checklist items that are explicitly required are still violated and these are considered critical blockers for this task. Specifically, the header logo uses a non‑BEM class logo-img in src/index.html and related SCSS (e.g., _card.scss, _header.scss), which breaks [CHECKLIST ITEM #1: All components follow BEM and use SCSS]; it should be renamed to something like header__logo / header__logo-img and styled as part of the header block. Multiple special colors are hard‑coded instead of using SCSS variables, especially #34568b (already defined as $color-title-hover) and repeated #fff card/button backgrounds across _card.scss, _cards-container.scss, _header.scss, and _stars.scss, which violates [CHECKLIST ITEM #2: Repeated sizes and special colors are put to variables]; replace these literals with the corresponding variables. There are also repeated sizes (e.g., 200px, 48px, 40px) that should be extracted into SCSS variables wherever they are reused to fully satisfy the same checklist item.
From the hover behavior side, the requirement that "all changes on :hover are smooth" is only partially met: some hover transitions are defined, but durations like 0.3s are duplicated and not centralized, and certain hover‑affected properties (like all card hover changes in _cards-container.scss and _card.scss) are not consistently covered; introducing a shared $transition-duration (e.g., 0.3s) and applying it to all hover transitions will make this requirement clearly satisfied. Finally, the BackstopJS visual tests for the card, main tag, and entire document are failing, which strongly suggests the current card implementation differs from the expected catalog layout and should be rechecked against the mockup and updated until the visual diffs pass. Once you fix the BEM naming for the logo, replace hard‑coded colors/sizes with SCSS variables, standardize hover transitions, and ensure the card/catalog visuals match the reference, your solution will align well with the task and be ready for approval. I encourage you to keep your current good structure (semantic HTML, grid layout, and most hover behaviors) and treat these changes as polishing steps to fully meet the project standards.
✨ 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
| <img | ||
| src="./images/logo.png" | ||
| alt="Moyo logo" | ||
| class="logo-img" |
There was a problem hiding this comment.
This violates checklist item "All components follow BEM and use SCSS" — the logo-img class is not a BEM-style element of the header block. Consider renaming it to something like header__logo or header__logo-image to match BEM.
| > | ||
| <img | ||
| src="./images/logo.png" | ||
| alt="Moyo logo" |
There was a problem hiding this comment.
This violates checklist item "Repeated sizes and special colors are put to variables" — color: #34568b; is a hard-coded special color already defined as $color-title-hover in _variables.scss, so this should use that variable instead of the literal value.
|
|
||
| .card:hover & { | ||
| color: #34568b; | ||
| } |
There was a problem hiding this comment.
This violates checklist item "All components follow BEM and use SCSS" because class="logo-img" is not a BEM-style element or modifier of a block (it should be something like header__logo-img attached to the header block or a dedicated logo block).
| transition: color 0.3s; | ||
|
|
||
| .card:hover & { | ||
| color: #34568b; |
There was a problem hiding this comment.
This violates checklist item "Repeated sizes and special colors are put to variables"; the hover color #34568b is already defined as $color-title-hover in _variables.scss and should be used instead of the hardcoded literal.
| border-radius: 5px; | ||
| padding: 32px 16px 16px; | ||
| background: #fff; | ||
| transition: transform 0.3s; |
There was a problem hiding this comment.
This partially violates checklist item "All changes on :hover are smooth"; although you have transitions on transform and button color/background, consider centralizing the 0.3s duration into a SCSS variable (e.g., $transition-duration) and ensuring all hover-affected properties across components use it consistently.
| margin: 0; | ||
| } | ||
|
|
||
| ul { |
There was a problem hiding this comment.
This violates checklist item "Repeated sizes and special colors are put to variables" — card background #fff is a repeated special color and should be replaced with an SCSS variable (similar to $color-main, $color-text-accent, etc.).
| height: 40px; | ||
| width: 40px; |
There was a problem hiding this comment.
This violates checklist item "Repeated sizes and special colors are put to variables" — you already have $color-title-hover defined, so the title hover color should use that variable instead of the hard-coded #34568b.
| display: block; | ||
| height: 40px; | ||
| width: 40px; | ||
| } |
There was a problem hiding this comment.
This violates checklist item "All components follow BEM and use SCSS" — the logo-img class in HTML is not aligned with your BEM naming (it should be something like header__logo or similar) and styled via SCSS as part of the Header block.
| height: 16px; | ||
| margin-right: 4px; | ||
| background-image: url('../images/star.svg'); | ||
| background-repeat: no-repeat; |
There was a problem hiding this comment.
This violates checklist item "Repeated sizes and special colors are put to variables" — card background #fff should be represented by a SCSS variable (similar to other colors) rather than a hard-coded literal.
| &__star { | ||
| width: 16px; | ||
| height: 16px; | ||
| margin-right: 4px; | ||
| background-image: url('../images/star.svg'); | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
|
|
||
| &:last-child { |
There was a problem hiding this comment.
This violates checklist item "Repeated sizes and special colors are put to variables" — sizes like 200px, 48px, and 40px are repeated across the layout and should be extracted into SCSS variables and used here instead of hard-coded literals.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one explicit checklist requirement is still violated and treated as critical for this task. In _header.scss, you hard‑code special colors (#fff, #060b35, and #00acdc via --primary-blue) instead of using the SCSS variables from _variables.scss ($color-white, $color-text-accent, $color-main), which breaks [CHECKLIST ITEM #2: Repeated sizes and special colors are put to variables]. To fix this, @use 'variables' as *; in _header.scss and replace those literals with the corresponding SCSS variables; if you need a CSS custom property like --primary-blue, derive it from $color-main rather than duplicating the hex value. There is also a minor SCSS linting/style issue in src/styles/_cards-container.scss around line 10 where there is a newline after the * operator; adjust that line so the * and the following token are on the same line to satisfy scss/operator-no-newline-after. Functionally your layout, BEM structure, hovers, grid breakpoints, and data-qa attributes look solid, so once you centralize the remaining header colors into variables and fix that small linter complaint, your solution should be ready. Keep going—you're very close to a fully compliant submission.
✨ 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
| <body> | ||
| <h1>Catalog</h1> | ||
| <header class="header"> | ||
| <a | ||
| href="#" | ||
| class="header__logo-link" | ||
| > | ||
| <img | ||
| src="./images/logo.png" | ||
| alt="Moyo logo" | ||
| class="header__logo-img" | ||
| /> | ||
| </a> | ||
|
|
||
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#apple" | ||
| class="nav__link is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#samsung" | ||
| class="nav__link" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#smartphones" | ||
| class="nav__link" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#laptops" | ||
| class="nav__link" | ||
| data-qa="nav-hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#gadgets" | ||
| class="nav__link" | ||
| > | ||
| Gadgets | ||
| </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> |
There was a problem hiding this comment.
This violates checklist item #2: "Repeated sizes and special colors are put to variables" because header colors are still hard-coded (e.g., #fff for background, #060b35 for nav links, and #00acdc via --primary-blue) instead of using the SCSS variables from _variables.scss like $color-white, $color-text-accent, and $color-main. Consider @use 'variables' as *; in this file and replacing these literals with the corresponding variables to keep colors centralized.
| grid-template-columns: repeat(3, $card-width); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This violates checklist item #2: "Repeated sizes and special colors are put to variables" because background-color: #fff; hard-codes the same white color you already defined as $color-white in _variables.scss; use the SCSS variable here instead of the literal.
| height: 60px; | ||
| box-sizing: border-box; | ||
|
|
||
| background-color: #fff; |
There was a problem hiding this comment.
This still hard-codes #fff for the header background, even though $color-white is defined in _variables.scss. This violates checklist item: Repeated sizes and special colors are put to variables; consider using your SCSS color variable instead of the literal here.
| @@ -0,0 +1,95 @@ | |||
| :root { | |||
| --primary-blue: #00acdc; | |||
There was a problem hiding this comment.
Defining --primary-blue: #00acdc; duplicates the $color-main SCSS variable value instead of reusing it, which goes against the checklist item Repeated sizes and special colors are put to variables. Try relying on the existing SCSS variable (or deriving the CSS variable from it) so the color is not hard-coded twice.
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
:hoverare smoothCode follows all the Code Style Rules ❗️
DEMO LINK
TEST REPORT LINK