task test#6294
Conversation
davidmonn
commented
Jun 30, 2026
davidmonn
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
I am requesting changes because several core requirements and test-critical attributes are still missing, which block the task from being considered complete. The implementation does not yet add class="is-active" to the first nav link, data-qa="nav-hover" to the 4th nav link, or data-qa="card" / data-qa="card-hover" to the first card and its Buy button, and the visual tests explicitly fail due to these missing elements. The catalog grid is hard-coded to 4 columns with gap: 48px and width: 1200px, and cards are 198px wide; this violates the required responsive 1/2/3/4-column behavior, fixed 200px card width, vertical gap 46px, and catalog paddings 50px vertical and 40px horizontal. Hover behavior and transitions are incomplete: cards do not scale by 20%, the card title (use .card__title or adapt your existing class) does not change to #34568b on hover, nav link base color is #000 instead of #060b35, and buttons do not switch background/text colors with a 300ms transition, all of which are required hover effects [CHECKLIST ITEM #5].
Critical fixes to focus on:
- Update
index.htmlto addis-activeon the first nav link,data-qa="nav-hover"on the 4th nav link,data-qa="card"on the first card’s root element, anddata-qa="card-hover"on the first card’s Buy link. - Adjust
.catalogin your SCSS to use grid with media queries for 1/2/3/4 columns at the specified breakpoints, setpadding: 50px 40px, userow-gap: 46pxandcolumn-gap: 48px, and ensure.cardhaswidth: 200px. - Implement the hover rules and transitions:
transition: 0.3son cards, nav links, and buttons; scale cards by ~20% on:hover, change the card title color to#34568bon card hover, use base nav color#060b35and hover color#00acdc, and make.card__buttonhover tobackground: #fffandcolor: #00acdc.
Minor improvements include centralizing repeated colors/sizes into SCSS variables and removing or disabling legacy non-BEMindex.cssto rely solely on your SCSS+BEM blocks [CHECKLIST ITEM #1, #2]. You have a solid structural base; once these missing behaviors and attributes are added, your solution will align much better with the spec and pass the visual and selector-based tests.
✨ 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
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| apple |
There was a problem hiding this comment.
This navigation link should have the is-active class according to the description: add the class is-active to the first link (Apple) in the navigation. Also, all nav links need color: #060b35 as base color in CSS instead of black.
| <nav class="header__nav"> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > |
There was a problem hiding this comment.
Navigation link base color is set to #000, but the description specifies nav links should have #060b35 color. This conflicts with the requirement and should use the given color (ideally via a SCSS variable).
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| smartphones | ||
| </a> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > |
There was a problem hiding this comment.
Only the underline width transition is implemented here; there is no transition for link text color itself or for card/button hover behavior elsewhere. The description requires that navigation link text color change to #00acdc on hover and that all changes on hover are smooth (300ms). You should add appropriate transition properties for color and other hover effects to satisfy checklist item #5: All changes on :hover are smooth.
| display: flex; | ||
| height: 18px; | ||
| gap: 85px; | ||
| margin-bottom: 16px; | ||
| } | ||
|
|
||
| .card__price-label { | ||
| color: $secondary; | ||
| font-weight: 400; | ||
| font-size: 12px; | ||
| line-height: 18px; | ||
| } | ||
|
|
||
| .card__price-value { | ||
| font-weight: 700; | ||
| font-size: 16px; | ||
| line-height: 18px; | ||
| } | ||
|
|
||
| .card__button { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
|
|
||
| width: 166px; | ||
| height: 40px; | ||
|
|
||
| color: #fff; | ||
| background-color: $blue-accent; | ||
|
|
||
| text-decoration: none; | ||
| text-transform: uppercase; | ||
|
|
||
| font-weight: 700; | ||
| font-size: 14px; | ||
|
|
||
| line-height: 16px; |
There was a problem hiding this comment.
The first card should have data-qa="card", and its Buy link should have data-qa="card-hover", plus the card title class referenced in the description is .card__title for hover color change; currently these attributes and the title class are missing, so tests relying on them will fail.
| .catalog { | ||
| display: grid; | ||
| width: 1200px; | ||
| margin: auto; | ||
| gap: 48px; | ||
|
|
||
| margin-top: 51px; | ||
|
|
||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(2, 408px); | ||
|
|
||
| align-items: center; | ||
| justify-content: center; | ||
| } |
There was a problem hiding this comment.
The .catalog grid needs separate vertical gap of 46px and responsive grid-template-columns with breakpoints (1,2,3,4 columns) instead of a fixed 4-column layout; also paddings 50px vertical and 40px horizontal are required for the catalog container.
| .card { | ||
| display: flex; | ||
| flex-direction: column; | ||
|
|
||
| width: 198px; | ||
| height: 406px; | ||
|
|
||
| background-color: #fff; | ||
| border: 1px solid $gray-elements; | ||
| border-radius: 5px; | ||
|
|
||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
Card width must be 200px according to the description; using 198px violates that requirement, and hover behavior (20% size increase with smooth 300ms transition) for .card is not implemented here.
| .card__button { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
|
|
||
| width: 166px; | ||
| height: 40px; | ||
|
|
||
| color: #fff; | ||
| background-color: $blue-accent; | ||
|
|
||
| text-decoration: none; | ||
| text-transform: uppercase; | ||
|
|
||
| font-weight: 700; | ||
| font-size: 14px; | ||
|
|
||
| line-height: 16px; | ||
|
|
||
| border-radius: 5px; | ||
| } |
There was a problem hiding this comment.
Button hover behavior should smoothly change background to #fff and text color to #00acdc over 300ms, but there is no transition declared and no :hover styles for .card__button, which violates checklist item #5 and the hover requirements.
| display: block; | ||
| background-image: url('../images/imac.jpeg'); | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
|
|
||
| width: 160px; | ||
| height: 134px; | ||
|
|
There was a problem hiding this comment.
Nav links should use base color #060b35 and have a smooth 300ms color transition to #00acdc on hover; currently they are #000 with no transition, so this does not meet the description or checklist item #5.
| margin-right: 16px; | ||
| margin-left: 16px; | ||
| } | ||
|
|
||
| .card__description-text { | ||
| margin-bottom: 4px; | ||
|
|
||
| font-weight: 500; | ||
| font-size: 12px; | ||
|
|
||
| line-height: 18px; |
There was a problem hiding this comment.
To fully satisfy hover requirements, you need a 300ms transition for the underline as well, and also a color transition on .header__link; right now only the pseudo-element’s width transition is defined.
| position: relative; | ||
| box-sizing: border-box; | ||
| display: flex; | ||
| color: #000; | ||
| text-decoration: none; | ||
| line-height: 60px; |
There was a problem hiding this comment.
The first navigation link (Apple) must have the is-active class according to the description: "add the class is-active to the first link (Apple) in the navigation". Also, the 4th nav link (Laptops & computers) must include data-qa="nav-hover", which is currently missing.
| height: 4px; | ||
| background: $blue-accent; | ||
| border-radius: 8px; | ||
| transition: width 0.3s; | ||
| } | ||
|
|
There was a problem hiding this comment.
The 4th navigation link (Laptops & computers) needs data-qa="nav-hover" as required: "add data-qa="nav-hover" (not just hover) to the 4th nav link for testing". This attribute is not present on this anchor.
| .stars { | ||
| display: flex; | ||
|
|
||
| &__star { | ||
| width: 16px; | ||
| 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 “Cards are shown in 1, 2, 3, or 4 columns based on screen resolution” – the catalog grid is hardcoded to 4 columns and does not use media queries to switch between 1/2/3/4 columns as required.
|
|
||
| &--1 &__star:nth-child(-n + 1), | ||
| &--2 &__star:nth-child(-n + 2), | ||
| &--3 &__star:nth-child(-n + 3), | ||
| &--4 &__star:nth-child(-n + 4), |
There was a problem hiding this comment.
This violates the description requirement: “cards have fixed width - 200px” – card width is set to 198px instead of 200px.
| .stars { | ||
| display: flex; | ||
|
|
||
| &__star { | ||
| width: 16px; |
There was a problem hiding this comment.
This violates the description requirement: “the gap between cards should be - 48px horizontally and 46px vertically” – using a single gap: 48px sets both row and column gaps to 48px instead of 48px horizontal and 46px vertical (e.g., gap: 46px 48px).
| .stars { | ||
| display: flex; | ||
|
|
||
| &__star { | ||
| width: 16px; | ||
| height: 16px; | ||
| margin-right: 4px; | ||
| background-image: url(./../images/star.svg); | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
|
|
||
| &:last-child { | ||
| margin-right: 0; | ||
| } |
There was a problem hiding this comment.
This violates the description requirement: “cards container(catalog) have fixed paddings (50px vertically and 40px horizontally)” – no padding is set on .catalog.
| .header__nav__container--menu { | ||
| display: flex; | ||
| flex-direction: row; | ||
| background-color: red; | ||
| height: 60px; | ||
| align-items: center; |
There was a problem hiding this comment.
This violates the description requirement: “add the class is-active to the first link (Apple) in the navigation” – the first <a> does not include is-active in its class list.
| display: flex; | ||
| gap: 20px; | ||
| text-transform: uppercase; | ||
| } | ||
|
|
||
| .header__nav--link { |
There was a problem hiding this comment.
This violates the description requirement: “add data-qa="nav-hover" (not just hover) to the 4th nav link for testing (Laptops & computers)” – the 4th <a> lacks the required data-qa attribute.
| .main__cards--container { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(2, 408px); | ||
| background-color: rgba(0, 0, 255, 0.55); | ||
| } | ||
|
|
||
| .main__card__product__container { | ||
| display: flex; | ||
| background-color: yellow; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| } | ||
|
|
There was a problem hiding this comment.
| display: block; | ||
| background-image: url('../images/imac.jpeg'); | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| width: 160px; |
There was a problem hiding this comment.
This violates the description requirement: “cards have fixed width - 200px” – .card width is set to 198px instead of 200px.
| .main__cards--container { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(2, 408px); | ||
| background-color: rgba(0, 0, 255, 0.55); |
There was a problem hiding this comment.
This violates the description requirement: “the gap between cards should be - 48px horizontally and 46px vertically” – setting a single gap: 48px applies 48px to both row and column gaps, instead of 46px vertical and 48px horizontal.
| .main__cards--container { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(2, 408px); | ||
| background-color: rgba(0, 0, 255, 0.55); | ||
| } | ||
|
|
||
| .main__card__product__container { | ||
| display: flex; | ||
| background-color: yellow; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| } | ||
|
|
There was a problem hiding this comment.
This violates the description requirement: “cards container(catalog) have fixed paddings (50px vertically and 40px horizontally)” – .catalog does not define any padding.
| display: block; | ||
| background-image: url('../images/imac.jpeg'); | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| width: 160px; | ||
| height: 134px; | ||
| margin-bottom: 40px; | ||
| } | ||
|
|
||
| .main__card__desc { | ||
| font-weight: 500; | ||
| font-size: 12px; | ||
| line-height: 18px; |
There was a problem hiding this comment.
This violates checklist item #5: “All changes on :hover are smooth” and the description’s hover requirements – .card lacks transition properties and :hover rules to increase size by 20% and to change the card title color to #34568b (e.g. .card:hover .card__title).
|
|
||
| .header__nav__container--menu { | ||
| display: flex; | ||
| flex-direction: row; | ||
| background-color: red; |
There was a problem hiding this comment.
This violates the description requirement: “nav links color is not black anymore (nav links should have #060b35 color)” – base color is set to #000 instead of #060b35.
| .header__nav--logo { | ||
| display: flex; |
There was a problem hiding this comment.
This partially violates checklist item #5: “All changes on :hover are smooth” – although hover color uses $blue-accent (#00acdc), there is no transition on color, only on the pseudo-element width, so the text color change is not smooth over 300ms as required.
| .main__cards--container { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(2, 408px); | ||
| background-color: rgba(0, 0, 255, 0.55); | ||
| } | ||
|
|
||
| .main__card__product__container { | ||
| display: flex; | ||
| background-color: yellow; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .main__card--product--image { | ||
| display: block; | ||
| background-image: url('../images/imac.jpeg'); | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| width: 160px; | ||
| height: 134px; | ||
| margin-bottom: 40px; | ||
| } | ||
|
|
||
| .main__card__desc { | ||
| font-weight: 500; | ||
| font-size: 12px; | ||
| line-height: 18px; | ||
| margin-bottom: 4px; | ||
| } | ||
|
|
||
| .header__nav__container--menu { | ||
| display: flex; | ||
| flex-direction: row; | ||
| background-color: red; | ||
| height: 60px; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .header__nav--logo { | ||
| display: flex; | ||
| background-image: url('../images/logo.png'); | ||
| width: 40px; | ||
| height: 40px; | ||
| margin-left: 50px; | ||
| margin-right: 419px; | ||
| } | ||
|
|
||
| .header__nav--nav { | ||
| display: flex; | ||
| gap: 20px; | ||
| text-transform: uppercase; | ||
| } | ||
|
|
||
| .header__nav--link { | ||
| display: flex; | ||
| color: #000; | ||
| text-decoration: none; | ||
| line-height: 60px; | ||
| } | ||
|
|
||
| body { | ||
| margin: 0; | ||
| font-family: Roboto, Arial, Helvetica, sans-serif; | ||
| font-weight: 500; | ||
| font-style: 12px; | ||
| } | ||
|
|
||
| /*# sourceMappingURL=index.css.map */ |
There was a problem hiding this comment.
This old CSS uses non-BEM class names and appears to be from a previous implementation; since the description says to rewrite blocks using BEM and SCSS, continuing to rely on these styles would violate checklist item #1: “All components follow BEM and use SCSS.” Consider removing or ensuring it is unused.
| .main__cards--container { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, 200px); |
There was a problem hiding this comment.
The color #00acdc is already stored in $blue-accent, but other special colors and repeated sizes (like #060b35, #34568b, card width, gaps, paddings) should also be moved into SCSS variables to fully satisfy checklist item #2: “Repeated sizes and special colors are put to variables.”
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 several core task requirements are still missing and these are blocking completion. The catalog grid is fixed to 4 columns with incorrect gaps and no media queries, so it does not show 1/2/3/4 columns at the required breakpoints and lacks the specified padding: 50px 40px on .catalog [CHECKLIST ITEM #3, #4]. The first nav link (Apple) in index.html still does not have class="is-active", which is explicitly required and used by the tests, and there is no visual style for the active state. Card hover behavior is not implemented: .card has no transition or :hover transform to scale by ~20%, and there is no rule like .card:hover .card__description-text { color: #34568b; }, so the required smooth card and title color change are missing [CHECKLIST ITEM #5]. The button (.card__button) also lacks hover styles and a 300ms transition to switch its background to #fff and text color to #00acdc, which is another core hover requirement. In addition, the legacy non-BEM src/styles/index.css is still present and can conflict with your SCSS/BEM solution, and some repeated colors like #fff are not yet moved into SCSS variables [CHECKLIST ITEM #1, #2].
To fix this, update index.html so the first nav link has class="header__link is-active", refactor .catalog in SCSS to use grid with row-gap: 46px, column-gap: 48px, padding: 50px 40px, and media queries at 488/768/1024px to show 1/2/3/4 columns, add transition: 300ms and transform: scale(1.2) (or similar) on .card:hover plus a hover color rule for your title text, and implement .card__button:hover to swap background/text colors with a 300ms transition. Also, stop using or remove index.css and introduce a $white (and other needed) variables so repeated colors are not hard-coded. You’ve made solid progress with the structure and BEM/SCSS setup; once these behaviors and layout details are corrected, your solution should align well with the spec and pass the tests.
✨ 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
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| apple |
There was a problem hiding this comment.
This violates the requirement: "add the class is-active to the first link (Apple) in the navigation". The first a.header__link currently has no is-active class, so tests relying on that selector will fail.
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| </div> | ||
|
|
||
| <div class="card__review-count">Reviews: 5</div> | ||
| </div> | ||
|
|
||
| <div class="card__price"> | ||
| <div class="card__price-label">Price:</div> | ||
|
|
||
| <div class="card__price-value">$2,199</div> | ||
| </div> | ||
|
|
||
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="card-hover" | ||
| > | ||
| Buy | ||
| </a> |
There was a problem hiding this comment.
This violates checklist item #5: "All changes on :hover are smooth" and the requirement to "change the button background to #fff and text color to #00acdc on hover". .card__button has no transition and no :hover rule to update background and text color, so the button hover effect is missing.
| </a> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| data-qa="nav-hover" | ||
| > | ||
| laptops & computers | ||
| </a> |
There was a problem hiding this comment.
This violates the requirement to "change the card title text color to #34568b when the card is hovered (.card:hover .card__title)". While you have .card__description-text as the title text, there is no :hover rule on .card that targets this element to update its color.
| <nav class="header__nav"> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| apple | ||
| </a> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| samsung |
There was a problem hiding this comment.
This partially violates the requirement to make nav hover changes smooth in 300ms, and the previous correction to add is-active. You correctly set color: $nav-color and transition: 300ms, and :hover changes color to $blue-accent, but there is no styling for .header__link.is-active so the active state isn’t visually distinguished, and the requirement specifically calls for adding that class to the first link.
| } | ||
|
|
||
| .card__image { | ||
| display: block; |
There was a problem hiding this comment.
This link should have the is-active class according to the description: add the class is-active to the first link (Apple) in the navigation.
| .catalog { | ||
| display: grid; | ||
| width: 1200px; |
There was a problem hiding this comment.
The catalog grid is hard-coded to 4 columns and 2 rows with gap: 44px 48px and a fixed width 1200px; this violates the description and checklist item Cards are shown in 1, 2, 3, or 4 columns based on screen resolution and the required gaps/paddings (48px horizontally, 46px vertically, 50px vertical, 40px horizontal). You should use responsive grid-template-columns with media queries and the exact gap/padding values.
| .card { | ||
| display: flex; | ||
| flex-direction: column; | ||
|
|
||
| width: 200px; | ||
| height: 406px; | ||
|
|
||
| background-color: #fff; | ||
| border: 1px solid $gray-elements; | ||
| border-radius: 5px; | ||
|
|
||
|
|
||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
Hover behavior on cards is missing: there is no transition or :hover rule to scale the card by ~20% and change its title color. This violates the description requirement increase the card by 20 percent (neighboring cards should not be affected) and .card:hover .card__title color #34568b, as well as checklist item #5 (All changes on :hover are smooth).
| .card__button { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
|
|
||
| width: 166px; | ||
| height: 40px; | ||
|
|
||
| color: #fff; | ||
| background-color: $blue-accent; | ||
|
|
||
| text-decoration: none; | ||
| text-transform: uppercase; | ||
|
|
||
| font-weight: 700; | ||
| font-size: 14px; | ||
|
|
||
| line-height: 16px; | ||
|
|
||
| border-radius: 5px; | ||
| } |
There was a problem hiding this comment.
The button has no hover styles or transition; per the description and checklist item #5, on hover its background must change to #fff and text color to #00acdc with a 300ms transition.
| .card__description-text { | ||
| margin-bottom: 4px; | ||
|
|
||
| font-weight: 500; | ||
| font-size: 12px; | ||
|
|
||
| line-height: 18px; | ||
| } |
There was a problem hiding this comment.
There is no hover style that changes the card title text color to #34568b when the card is hovered. Even if you use .card__description-text instead of .card__title, you still need a rule like .card:hover .card__description-text { color: #34568b; } to satisfy the description.
| position: relative; | ||
| box-sizing: border-box; | ||
| display: flex; | ||
| color: $nav-color; | ||
| text-decoration: none; | ||
| line-height: 60px; |
There was a problem hiding this comment.
This a element for the first nav item (apple) is missing the required is-active class. This violates the description requirement and previous HIGH issue: "add the class is-active to the first link (Apple) in the navigation".
| .header { | ||
| display: flex; | ||
| flex-direction: row; | ||
| box-shadow: 0 2px 4px #0000000d; | ||
|
|
||
| height: 60px; | ||
|
|
||
| justify-content: space-between; | ||
| align-items: center; | ||
|
|
||
| &__logo { | ||
| display: flex; | ||
| background-image: url('../images/logo.png'); | ||
|
|
There was a problem hiding this comment.
The catalog container is required to have fixed paddings of 50px vertically and 40px horizontally and to use a responsive grid (1/2/3/4 columns at 0/488/768/1024px). Currently it only has margin: auto, margin-top: 51px, width: 1200px, and a hard-coded 4-column grid. This violates: "cards container(catalog) have fixed paddings (50px vertically and 40px horizontally)" and checklist item #4: "Cards are shown in 1, 2, 3, or 4 columns based on screen resolution".
| height: 40px; | ||
|
|
||
| margin-left: 50px; | ||
| margin-right: 419px; | ||
| } | ||
|
|
||
| &__nav { | ||
| display: flex; | ||
| margin-right: 50px; | ||
| gap: 20px; | ||
|
|
||
| text-transform: uppercase; | ||
| } | ||
| } |
There was a problem hiding this comment.
To meet the hover behavior requirements, the card itself should smoothly scale by about 20% on :hover, with neighboring cards unaffected, and the inner title (.card__description-text, as your title) should change color to #34568b when the card is hovered. Currently no transition or :hover styles exist on .card, and no rule like .card:hover .card__description-text is defined, violating: "increase the card by 20 percent" and "change the card title text color to #34568b when the card is hovered (.card:hover .card__title)" as well as checklist item #5: "All changes on :hover are smooth".
| .header { | ||
| display: flex; | ||
| flex-direction: row; | ||
| box-shadow: 0 2px 4px #0000000d; | ||
|
|
There was a problem hiding this comment.
The catalog grid gaps must be 48px horizontally and 46px vertically, but here you use gap: 44px 48px;. This violates the description: "the gap between cards should be - 48px horizontally and 46px vertically".
| align-items: center; | ||
|
|
||
| &__logo { | ||
| display: flex; | ||
| background-image: url('../images/logo.png'); | ||
|
|
||
| width: 40px; | ||
| height: 40px; | ||
|
|
||
| margin-left: 50px; | ||
| margin-right: 419px; | ||
| } | ||
|
|
There was a problem hiding this comment.
Card width is correctly 200px, but you also set a fixed height: 406px; and hard-code grid-template-rows: repeat(2, 408px);, while the task only specifies a fixed width and expects cards to flow in a responsive grid. Consider removing the fixed grid row definition and relying on card content height so the responsive layout works properly.
| } | ||
|
|
||
| &__nav { | ||
| display: flex; |
There was a problem hiding this comment.
You already use SCSS variables for $blue-accent, $secondary, etc., but background-color: #fff; and color: #fff; appear as raw values. To fully satisfy checklist item #2: "Repeated sizes and special colors are put to variables", consider adding a $white variable and using it consistently instead of hard-coded #fff.
| .stars { | ||
| display: flex; | ||
|
|
||
| &__star { | ||
| width: 16px; | ||
| height: 16px; | ||
| margin-right: 4px; | ||
| background-image: url(./../images/star.svg); | ||
| background-repeat: no-repeat; | ||
| background-position: center; |
There was a problem hiding this comment.
The .catalog grid is hard-coded to 4 columns with fixed width and wrong gap values. This violates checklist item #4: "Cards are shown in 1, 2, 3, or 4 columns based on screen resolution" and the description requirement for 1/2/3/4 columns using media queries at 488px, 768px, and 1024px. It also uses gap: 44px 48px instead of the required 46px vertical and 48px horizontal, and misses the required paddings 50px vertical, 40px horizontal.
| .header__nav__container--menu { | ||
| display: flex; | ||
| flex-direction: row; | ||
| background-color: red; | ||
| height: 60px; |
There was a problem hiding this comment.
The first nav link (apple) does not have class="is-active". This violates the description requirement: "add the class is-active to the first link (Apple) in the navigation".
| .main__cards--container { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(2, 408px); | ||
| background-color: rgba(0, 0, 255, 0.55); |
There was a problem hiding this comment.
.catalog uses gap: 44px 48px; instead of the required 46px vertical and 48px horizontal gap. This violates the description: "the gap between cards should be - 48px horizontally and 46px vertically".
| .main__cards--container { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(2, 408px); | ||
| background-color: rgba(0, 0, 255, 0.55); | ||
| } | ||
|
|
||
| .main__card__product__container { | ||
| display: flex; | ||
| background-color: yellow; |
There was a problem hiding this comment.
The grid is hard-coded as grid-template-columns: repeat(4, 200px); and grid-template-rows: repeat(2, 408px); without media queries, so the number of columns does not change by screen resolution. This violates checklist item #4 and the description’s required responsive behavior (1/2/3/4 columns).
| display: block; | ||
| background-image: url('../images/imac.jpeg'); | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| width: 160px; | ||
| height: 134px; | ||
| margin-bottom: 40px; | ||
| } | ||
|
|
||
| .main__card__desc { | ||
| font-weight: 500; | ||
| font-size: 12px; | ||
| line-height: 18px; | ||
| margin-bottom: 4px; |
There was a problem hiding this comment.
.card defines base layout and width but no transition or :hover styles to scale the card by 20% and change the title text color. This violates the hover requirements: "increase the card by 20 percent" and "change the card title text color to #34568b when the card is hovered", as well as checklist item #5: "All changes on :hover are smooth".
| .header__nav--nav { | ||
| display: flex; | ||
| gap: 20px; | ||
| text-transform: uppercase; | ||
| } | ||
|
|
||
| .header__nav--link { | ||
| display: flex; |
There was a problem hiding this comment.
The card title (.card__description-text) does not have a hover rule to change its color when the card is hovered. The description explicitly requires .card:hover .card__title to become #34568b; with your naming you should implement .card:hover .card__description-text { color: #34568b; } with a transition.
| .main__cards--container { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(2, 408px); | ||
| background-color: rgba(0, 0, 255, 0.55); | ||
| } | ||
|
|
||
| .main__card__product__container { | ||
| display: flex; | ||
| background-color: yellow; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .main__card--product--image { | ||
| display: block; | ||
| background-image: url('../images/imac.jpeg'); | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| width: 160px; | ||
| height: 134px; | ||
| margin-bottom: 40px; | ||
| } | ||
|
|
||
| .main__card__desc { | ||
| font-weight: 500; | ||
| font-size: 12px; | ||
| line-height: 18px; | ||
| margin-bottom: 4px; | ||
| } | ||
|
|
||
| .header__nav__container--menu { | ||
| display: flex; | ||
| flex-direction: row; | ||
| background-color: red; | ||
| height: 60px; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .header__nav--logo { | ||
| display: flex; | ||
| background-image: url('../images/logo.png'); | ||
| width: 40px; | ||
| height: 40px; | ||
| margin-left: 50px; | ||
| margin-right: 419px; | ||
| } | ||
|
|
||
| .header__nav--nav { | ||
| display: flex; | ||
| gap: 20px; | ||
| text-transform: uppercase; | ||
| } | ||
|
|
||
| .header__nav--link { | ||
| display: flex; | ||
| color: #000; | ||
| text-decoration: none; | ||
| line-height: 60px; | ||
| } | ||
|
|
||
| body { | ||
| margin: 0; | ||
| font-family: Roboto, Arial, Helvetica, sans-serif; | ||
| font-weight: 500; | ||
| font-style: 12px; | ||
| } | ||
|
|
||
| /*# sourceMappingURL=index.css.map */ |
There was a problem hiding this comment.
This entire file contains legacy non-BEM styles (main__cards--container, header__nav__container--menu, etc.) that are not used by your current HTML and conflict with the requirement to rewrite blocks using BEM and SCSS. The MEDIUM priority note says to "remove or disable legacy non-BEM index.css"; you should stop using this file.
| @import './utils/var'; | ||
| @import './blocs/card'; | ||
| @import './blocs/header'; | ||
| @import './blocs/stars'; | ||
|
|
||
| * { | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
The catalog container is required to use paddings (50px vertically and 40px horizontally). Currently, .catalog has margin: auto and margin-top: 51px, but no padding, which violates the description requirement for fixed paddings on the catalog container.
| @import './utils/var'; | ||
| @import './blocs/card'; | ||
| @import './blocs/header'; | ||
| @import './blocs/stars'; | ||
|
|
||
| * { | ||
| box-sizing: border-box; | ||
| margin: 0; | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
The catalog grid is hard-coded to 4 columns and 2 rows (grid-template-columns: repeat(4, 200px); grid-template-rows: repeat(2, 408px);). This violates checklist item #4: "Cards are shown in 1, 2, 3, or 4 columns based on screen resolution" and the description’s specified breakpoints (1 column default, 2 from 488px, 3 from 768px, 4 from 1024px). You need media queries to adjust grid-template-columns at those widths.
| @import './utils/var'; | ||
| @import './blocs/card'; | ||
| @import './blocs/header'; | ||
| @import './blocs/stars'; | ||
|
|
There was a problem hiding this comment.
The grid gap should be 48px horizontally and 46px vertically, but you currently use gap: 44px 48px;. This violates the description: "the gap between cards should be - 48px horizontally and 46px vertically".
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 refine your skills.
You’ve done a solid job setting up the HTML structure with header, main catalog, cards, stars, and moving most styles into BEM‑based SCSS with shared variables and data-qa attributes in the right places. The main areas to improve are: aligning the catalog grid breakpoints and padding with the spec (1/2/3/4 columns at 0/488/768/1024px with padding: 50px 40px and the required gaps) [CHECKLIST ITEM #3, #4], adding the is-active class and a visible style to the first nav link (Apple), and implementing the hover behaviors for cards (scale ~20% and title color change), and buttons (background/text color swap with 300ms transitions) [CHECKLIST ITEM #5]. It will also help to remove or disconnect the legacy index.css file so only your SCSS/BEM rules apply [CHECKLIST ITEM #1], and introduce a variable like $white instead of repeating #fff directly in multiple places [CHECKLIST ITEM #2].
Treat these issues as a checklist for your next iteration: adjust the grid and padding in .catalog, add and style .header__link.is-active, implement the required hover transitions on .card, .card__description-text, and .card__button, and centralize your repeated colors in var.scss. Your foundation is good, and with these refinements you’ll be much closer to a production‑ready, spec‑compliant solution.
✨ 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
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| apple |
There was a problem hiding this comment.
This violates the description requirement: add the class is-active to the first link (Apple) in the navigation – the first link currently only has class="header__link" and no is-active modifier or active styling.
| href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;500;700&display=swap" | ||
| rel="stylesheet" | ||
| /> | ||
|
|
||
| <link | ||
| rel="stylesheet" | ||
| href="styles/index.scss" |
There was a problem hiding this comment.
This media query at 300px conflicts with the required breakpoints from the description (488px, 768px, 1024px) and checklist item Cards are shown in 1, 2, 3, or 4 columns based on screen resolution; you need to adjust min-width values to match those exact thresholds.
| </head> | ||
|
|
||
| <body> | ||
| <h1>Catalog</h1> | ||
| <header class="header"> | ||
| <img | ||
| src="../src/images/logo.png" |
There was a problem hiding this comment.
The media query at 600px uses 2 columns but should match the spec: 2 columns starting at 488px and 3 from 768px; this mismatch violates checklist item Cards are shown in 1, 2, 3, or 4 columns based on screen resolution.
| class="header__logo" | ||
| /> | ||
|
|
||
| <nav class="header__nav"> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| apple | ||
| </a> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| samsung |
There was a problem hiding this comment.
The media queries at 1024px and 1200px conflict with the task description’s exact breakpoints and column counts. At 1024px you should have 4 columns, not 3, and there should be a 768px breakpoint for 3 columns, per checklist item Grid is used for the columns and Cards are shown in 1, 2, 3, or 4 columns based on screen resolution.
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| smartphones | ||
| </a> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| data-qa="nav-hover" | ||
| > | ||
| laptops & computers | ||
| </a> |
There was a problem hiding this comment.
The card hover behavior required by the description (increase the card by 20 percent, .card:hover .card__title color change, smooth 300ms) is missing; there is no transition on .card, no :hover transform, and no rule to update the title text color on hover [CHECKLIST ITEM #5].
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| smartphones | ||
| </a> | ||
| <a | ||
| class="header__link" |
There was a problem hiding this comment.
Hard-coding #fff here violates checklist item Repeated sizes and special colors are put to variables; consider introducing a $white (or similar) variable in var.scss and using it instead of the literal color.
| <p class="card__description-text"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </p> | ||
| <div class="card__code"> | ||
| <p class="card__description-code">Product code: 195434</p> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="card__reviews"> | ||
| <div class="stars stars--4"> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| </div> | ||
|
|
||
| <div class="card__review-count">Reviews: 5</div> | ||
| </div> | ||
|
|
||
| <div class="card__price"> |
There was a problem hiding this comment.
The button hover behavior required by the description (change the button background to #fff and text color to #00acdc on hover, smooth over 300ms) is not implemented; .card__button lacks transition and any :hover rule to swap colors [CHECKLIST ITEM #5].
| <nav class="header__nav"> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| apple | ||
| </a> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| samsung | ||
| </a> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > | ||
| smartphones | ||
| </a> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| data-qa="nav-hover" | ||
| > | ||
| laptops & computers | ||
| </a> | ||
| <a | ||
| class="header__link" | ||
| href="#" | ||
| > |
There was a problem hiding this comment.
Navigation link hover behavior is smooth and uses the correct color variable, but there is no style for the is-active state required in the description (first link Apple) – you need a visual style for active links so tests can detect it.
| grid-template-rows: repeat(2, 408px); | ||
| } | ||
| } | ||
|
|
||
| @media (min-width: 1200px) { | ||
| .catalog { |
There was a problem hiding this comment.
This violates the requirement: "add the class is-active to the first link (Apple) in the navigation" – tests rely on this class, and you also need to define a visual style for .header__link.is-active.
| .catalog { | ||
| display: grid; | ||
| width: 944px; | ||
| height: 862px; | ||
| margin: auto; | ||
| gap: 46px 48px; | ||
|
|
||
| padding-top: 50px; | ||
|
|
||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(1, 408px); |
There was a problem hiding this comment.
This violates the catalog layout requirements: the task and previous review require media queries at 488px, 768px, 1024px for 1/2/3/4 columns and padding: 50px 40px with row-gap: 46px, column-gap: 48px; here you use 300/600/1024/1200 breakpoints and only padding-top: 50px.
| width: 200px; | ||
| height: 406px; | ||
|
|
||
| background-color: #fff; |
There was a problem hiding this comment.
This uses a hard-coded #fff color instead of a SCSS variable, which breaks checklist item: "Repeated sizes and special colors are put to variables" – consider introducing $white (or similar) and using it here.
| .card { | ||
| display: flex; | ||
| flex-direction: column; | ||
|
|
||
| width: 200px; | ||
| height: 406px; | ||
|
|
||
| background-color: #fff; | ||
| border: 1px solid $gray-elements; | ||
| border-radius: 5px; | ||
|
|
||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
Card hover behavior is missing; checklist item "All changes on :hover are smooth" and the description require transition: 300ms on .card and a :hover transform (e.g. scale(1.2)) plus a title color change via .card:hover .card__description-text (or .card__title).
| .card__button { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
|
|
||
| width: 166px; | ||
| height: 40px; | ||
|
|
||
| color: #fff; | ||
| background-color: $blue-accent; | ||
|
|
||
| text-decoration: none; | ||
| text-transform: uppercase; | ||
|
|
||
| font-weight: 700; | ||
| font-size: 14px; | ||
|
|
||
| line-height: 16px; | ||
|
|
||
| border-radius: 5px; | ||
| } |
There was a problem hiding this comment.
Button hover behavior is missing: the task requires a 300ms transition and on :hover changing background to #fff and text color to #00acdc for .card__button [CHECKLIST ITEM #5]. Also, #fff should be moved to a variable per the checklist.
| position: relative; | ||
| box-sizing: border-box; | ||
| display: flex; | ||
| color: $nav-color; | ||
| text-decoration: none; |
There was a problem hiding this comment.
The first nav link (apple) is missing the required is-active class; this violates the description requirement: "add the class is-active to the first link (Apple) in the navigation" and the previous HIGH severity correction about active state.
| .header { | ||
| display: flex; | ||
| flex-direction: row; | ||
| box-shadow: 0 2px 4px #0000000d; | ||
|
|
||
| margin: auto; | ||
|
|
||
| width: 100%; |
There was a problem hiding this comment.
| height: 40px; | ||
|
|
||
| margin-left: 50px; | ||
| } | ||
|
|
||
| &__nav { | ||
| display: flex; | ||
| margin-right: 50px; | ||
|
|
||
| width: 100%; | ||
| height: 60px; | ||
| gap: 20px; | ||
|
|
||
| justify-content: flex-end; | ||
| text-transform: uppercase; | ||
| } | ||
| } | ||
|
|
||
| .header__link { | ||
| position: relative; | ||
| box-sizing: border-box; | ||
| display: flex; | ||
| color: $nav-color; | ||
| text-decoration: none; | ||
| line-height: 60px; | ||
| transition: 300ms; | ||
| font-size: 12px; | ||
| } | ||
|
|
||
| .header__link:hover { |
There was a problem hiding this comment.
| } | ||
|
|
||
| .header__link::before { | ||
| content: ''; | ||
| position: absolute; | ||
| left: 0; | ||
| bottom: -2px; | ||
| width: 0; | ||
| height: 4px; | ||
| background: $blue-accent; | ||
| border-radius: 8px; | ||
| transition: width 0.3s; | ||
| } |
There was a problem hiding this comment.
.card lacks transition: 300ms and transform: scale(1.2) (or similar) on :hover, so the card does not smoothly increase by ~20% when hovered, violating the description requirement and checklist item #5 about smooth hover scaling and the previous HIGH correction for card hover behavior.
| } | ||
|
|
||
| .header__link { | ||
| margin: 0; | ||
| padding: 0; | ||
| gap: 0; | ||
| font-size: 0.3em; | ||
| } |
There was a problem hiding this comment.
.card__description-text (your title text) does not change color on card hover; you need a rule like .card:hover .card__description-text { color: #34568b; } to satisfy "change the card title text color to #34568b when the card is hovered" from the description and checklist item #5.
| .stars { | ||
| display: flex; | ||
|
|
||
| &__star { | ||
| width: 16px; | ||
| height: 16px; | ||
| margin-right: 4px; | ||
| background-image: url(./../images/star.svg); |
There was a problem hiding this comment.
To meet the grid requirements, .catalog should use the specified breakpoints 488px, 768px, and 1024px with 1/2/3/4 columns and explicit padding: 50px 40px instead of fixed widths/heights and padding-top; this violates checklist items Grid is used for the columns and Cards are shown in 1, 2, 3, or 4 columns based on screen resolution as defined in the description.
|
|
||
| &__star { |
There was a problem hiding this comment.
Fixed width and height on .catalog can prevent the grid from naturally accommodating 1/2/3/4 columns and required padding/gaps; consider removing these and using gaps/padding instead to match the layout spec.
| .main__cards--container { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, 200px); | ||
| grid-template-rows: repeat(2, 408px); | ||
| background-color: rgba(0, 0, 255, 0.55); | ||
| } | ||
|
|
||
| .main__card__product__container { | ||
| display: flex; | ||
| background-color: yellow; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .main__card--product--image { | ||
| display: block; | ||
| background-image: url('../images/imac.jpeg'); | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| width: 160px; | ||
| height: 134px; | ||
| margin-bottom: 40px; | ||
| } | ||
|
|
||
| .main__card__desc { | ||
| font-weight: 500; | ||
| font-size: 12px; | ||
| line-height: 18px; | ||
| margin-bottom: 4px; | ||
| } | ||
|
|
||
| .header__nav__container--menu { | ||
| display: flex; | ||
| flex-direction: row; | ||
| background-color: red; | ||
| height: 60px; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .header__nav--logo { | ||
| display: flex; | ||
| background-image: url('../images/logo.png'); | ||
| width: 40px; | ||
| height: 40px; | ||
| margin-left: 50px; | ||
| margin-right: 419px; | ||
| } | ||
|
|
||
| .header__nav--nav { | ||
| display: flex; | ||
| gap: 20px; | ||
| text-transform: uppercase; | ||
| } | ||
|
|
||
| .header__nav--link { | ||
| display: flex; | ||
| color: #000; | ||
| text-decoration: none; | ||
| line-height: 60px; | ||
| } | ||
|
|
||
| body { | ||
| margin: 0; | ||
| font-family: Roboto, Arial, Helvetica, sans-serif; | ||
| font-weight: 500; | ||
| font-style: 12px; | ||
| } | ||
|
|
||
| /*# sourceMappingURL=index.css.map */ |
There was a problem hiding this comment.
This legacy non‑BEM CSS conflicts with the current requirement that "All components follow BEM and use SCSS" and the previous review’s instruction to remove/stop using src/styles/index.css; consider deleting this file or ensuring it is not used by the build.
| @import './utils/var'; | ||
| @import './blocs/card'; | ||
| @import './blocs/header'; | ||
| @import './blocs/stars'; |
There was a problem hiding this comment.
You’ve correctly centralized some colors as variables, but there is no variable for white (e.g. $white) even though #fff is used repeatedly in SCSS; this violates checklist item "Repeated sizes and special colors are put to variables".
| @import './utils/var'; | ||
| @import './blocs/card'; | ||
| @import './blocs/header'; | ||
| @import './blocs/stars'; | ||
|
|
||
| * { | ||
| box-sizing: border-box; | ||
| margin: 0; | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
The global reset and imports look good, but be sure you are not relying on index.css anymore so that only this SCSS entry point is used, in line with CHECKLIST ITEM #1.
raulriato
left a comment
raulriato
left a comment
There was a problem hiding this comment.
Your DEMO LINK and TEST REPORT LINK are not working.
Please, run npm run deploy and make sure the deploy ran successfully. Also, make sure both links work before re-requesting review.
3 participants
