solution: layout catalog#6300
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a few critical requirement violations that can break tests and core behavior. First, the data-qa attributes are over-applied: every card has data-qa="card" and every Buy link has data-qa="card-hover", but the task explicitly requires these only on the first card and its Buy link; this must be limited to that single card/link to avoid selector conflicts. Second, the selector for changing the title color on hover is incorrect: your nesting &:hover { &__title { ... } } compiles to .card:hover__title instead of .card:hover .card__title, so the title color never changes as required; adjust the SCSS so the generated selector is .card:hover .card__title. Third, [CHECKLIST ITEM #5] says all hover changes must be smooth over 300ms, but you only transition color (or transform) in several places while also changing background-color; you need to include background-color (or use transition: all 0.3s) on the card button and ensure all properties that change on hover are transitioned.
As smaller improvements, replace hardcoded 200px with your $card-width variable and remove the duplicated $color-white definition to fully satisfy the variables checklist, but these are secondary compared to the three points above. Once you fix the data-qa scope, the hover selector, and the missing transitions, your solution will align well with the specification and should pass the required checks.
✨ 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
| href="#" | ||
| class="nav__link" | ||
| data-qa="nav-hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Gadgets | ||
| </a> |
There was a problem hiding this comment.
Here, only color is transitioned, but the hover state also changes the background-color. To satisfy checklist item #5: “All changes on :hover are smooth” and the requirement that the button background change be smooth, include background-color in the transition (or transition all) so the button’s background change is animated as well.
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
According to the description, data-qa="card" should be added to the first card only, but here every card has data-qa="card". This violates that requirement and can break tests that rely on the first card’s unique selector.
| <a | ||
| href="#" | ||
| class="card__link" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
Similarly, data-qa="card-hover" is supposed to be on the link Buy inside the first card only, but all card__link elements have this attribute. This does not match the description requirement and may cause test issues.
| border-radius: 5px; | ||
| background-color: $color-blue-accent; | ||
| box-sizing: border-box; | ||
| transition: color 0.3s; |
There was a problem hiding this comment.
Checklist item #5 says All changes on :hover are smooth, but this transition only covers color. Since the hover state also changes background-color, that property will change instantly. Consider including background-color (or using transition: all 0.3s) so both color and background-color animate smoothly.
| justify-content: center; | ||
| padding: 50px 40px; | ||
| width: 100%; | ||
| margin: 0 auto; |
There was a problem hiding this comment.
Checklist item #5: “All changes on :hover are smooth” – here you only transition transform, but the card hover also affects nested content like title color; consider transitioning all properties that change (e.g., use transition: transform 0.3s, color 0.3s; or a general transition on the whole card).
| box-sizing: border-box; | ||
| } | ||
|
|
||
| *, |
There was a problem hiding this comment.
This hover nesting &:hover { &__title { ... } } compiles to .card:hover__title, not .card:hover .card__title, so the title color change on hover will not work and violates the explicit requirement .card:hover .card__title for changing title color.
|
|
||
| html, | ||
| body { | ||
| margin: 0; |
There was a problem hiding this comment.
Only transform is transitioned here; since the hover state is also meant to change the card title color, not all hover changes are smooth as required by checklist item #5 (“All changes on :hover are smooth”). Consider including all changing properties (or using transition: all 0.3s).
| @import './utils/mixin'; | ||
| @import './blocks/header'; | ||
| @import './blocks/catalog'; | ||
| @import './blocks/card'; | ||
| @import './blocks/stars'; | ||
|
|
||
| html, | ||
| body { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| } | ||
|
|
||
| *, |
There was a problem hiding this comment.
$color-white is defined a second time here. While SCSS will use the later value, duplicated variable definitions can be confusing; consider keeping a single definition for clarity and easier maintenance.
| // card | ||
| @mixin text-style($size, $weight: 400, $line-height: normal, $color: inherit) { | ||
| font-size: $size; | ||
| font-weight: $weight; | ||
| line-height: $line-height; |
There was a problem hiding this comment.
You defined $card-width but hardcode 200px in grid-template-columns instead of using the variable; this partially violates checklist item #2: “Repeated sizes and special colors are put to variables”, since you’re not actually using the variable for this repeated size.
|
|
||
| // card properties | ||
| $card-width: 200px; | ||
| $color-white: #fff; |
There was a problem hiding this comment.
The nesting &:hover { &__title { ... } } compiles to .card:hover__title instead of .card:hover .card__title, so the title color will not change when the card is hovered, contradicting the requirement .card:hover .card__title and breaking the hover title effect.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still two critical blockers that violate explicit requirements and will likely break automated tests. First, data-qa="card" and data-qa="card-hover" are applied to every .card and every Buy link in src/index.html (e.g., around lines 150–390), but the task clearly requires data-qa="card" only on the first card and data-qa="card-hover" only on the first card’s Buy link; this must be fixed by removing these attributes from all subsequent cards/links. Second, in src/styles/utils/variables.scss you define $color-white twice (once near the top for header, and again later for card styles), which conflicts with the checklist’s requirement to have a single source of truth for repeated colors; keep a single $color-white variable and reuse it in all blocks.
Everything else (grid layout with 1/2/3/4 columns, card width via $card-width, hover transitions on cards, buttons, and nav links, BEM/SCSS structure, and nav colors/active states) looks well implemented and aligned with the description. Once you restrict the data-qa attributes to only the first card and its Buy link, and remove the duplicate $color-white definition, your solution should fully satisfy the task and checklist. Keep up the good work—these last fixes are small but important for correctness and maintainability.
✨ 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
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
data-qa="card" is supposed to be applied only to the first card, but here it is also set on this second card; this violates the description and the previous HIGH-level note to limit data-qa="card" to the first card only.
| <a | ||
| href="#" | ||
| class="card__link" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
data-qa="card-hover" should be on the Buy link only in the first card, but this second card’s link also has it; this still violates the requirement to scope this data attribute to just the first card’s Buy link.
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
Same as above: this third card also has data-qa="card", but the description requires that attribute only on the first card.
| <a | ||
| href="#" | ||
| class="card__link" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
Same issue: this Buy link in the third card again uses data-qa="card-hover", which should be restricted to the first card only.
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
This fourth card again includes data-qa="card"; adjust so only the very first .card keeps this attribute.
| <a | ||
| href="#" | ||
| class="card__link" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
Same issue for data-qa="card-hover" on this Buy link; it must be limited to the first card only.
| background-color: $color-white; | ||
| border: 1px solid $color-gray-border; | ||
| border-radius: 5px; | ||
| width: $card-width; |
There was a problem hiding this comment.
This second declaration of $color-white duplicates the earlier one and violates checklist item #2 about variable reuse; keep a single $color-white variable definition instead of redefining it.
| &:last-child { | ||
| margin-right: 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
This second declaration of $color-white duplicates the earlier one at line 2; remove this duplicate to keep a single source of truth for the color and satisfy the variables checklist.
| box-sizing: border-box; | ||
| } | ||
|
|
||
| *, |
There was a problem hiding this comment.
This second declaration of $color-white duplicates the earlier one on line 2 and violates checklist item #2 about variables; keep a single $color-white definition and reuse it.
|
|
||
| // card properties | ||
| $card-width: 200px; | ||
| $color-white: #fff; |
There was a problem hiding this comment.
This second declaration of $color-white duplicates the earlier one at line 2. Checklist item #2 expects variables to be reused, not redefined; keep a single $color-white and remove this extra definition.
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 as an educational override so you can keep progressing while you practice, but there are two important issues to fix next.
You did a strong job on the overall layout and styling: the catalog uses a responsive grid with 1/2/3/4 columns, the cards have the correct fixed width and gaps, and hover effects on cards, nav links, and buttons are smooth and consistent with the requirements. The main blocking issue is in src/index.html: data-qa="card" and data-qa="card-hover" are currently added to every card and every Buy link, but the task explicitly requires these attributes only on the first card and its Buy link; please keep those attributes on the first card and remove them from all later cards. Also, the active nav underline (.nav__link.is-active::after) appears instantly; to fully meet the “all hover changes are smooth” requirement [CHECKLIST ITEM #5], add a transition (for example on opacity or transform) so its appearance/disappearance is animated as well.
Once you adjust the data-qa attributes and smooth out the active underline transition, your solution will not only pass tests but also closely match the intended behavior—great work so far, keep refining these last details.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
This violates the description requirement: data-qa="card" must be applied only to the first card. Here it’s used on a non-first card, so you need to remove data-qa="card" from all cards except the first one.
| <a | ||
| href="#" | ||
| class="card__link" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
This violates the description requirement: data-qa="card-hover" must be applied only to the Buy link inside the first card. Here it’s used on a non-first card’s link; remove data-qa="card-hover" from all Buy links except the first card’s.
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
Same issue as above: data-qa="card" must be present only on the first card; this non-first card should not have it.
| <a | ||
| href="#" | ||
| class="card__link" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
Same issue as above: data-qa="card-hover" should only be on the first card’s Buy link, not on later ones.
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
Same issue: data-qa="card" must be restricted to the first card only; remove it from this card.
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
Same issue: data-qa="card" should not be on this card; only the first card should carry that attribute.
| <a | ||
| href="#" | ||
| class="card__link" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
Same issue: data-qa="card-hover" must be only on the first Buy link; remove it here.
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
Same issue: data-qa="card" must be only on the first card; this later card should not have it.
| <a | ||
| href="#" | ||
| class="card__link" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
Same issue: data-qa="card-hover" must be only on the first card’s Buy link; remove it from this link as well.
| href="#" | ||
| class="nav__link" | ||
| data-qa="nav-hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a | ||
| href="#" |
There was a problem hiding this comment.
To fully satisfy checklist item #5 (“All changes on :hover are smooth”), consider adding a transition for the active underline (e.g., on opacity or transform) so the &.is-active::after visual change appears/disappears smoothly rather than instantly.
2 participants
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
:hoverare smoothCode follows all the Code Style Rules ❗️