add task solution - #6432
add task solution#6432evadab wants to merge 3 commits into
Conversation
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 not met and must be fixed before approval. Specifically, [CHECKLIST ITEM #2] requires repeated sizes to be moved to variables: you defined $star-size: 16px in src/styles/utils/_variables.scss, but src/styles/blocks/_stars.scss still uses hardcoded 16px for the star width/height (e.g., lines with width: 16px; height: 16px;). This violates the checklist and leads to inconsistency. Please replace those hardcoded values with $star-size and ensure the stars use the variable.
Additionally, ensure the card hover scale does not affect neighboring items as required: apply the transform to the card element only, set transform-origin: center, and consider will-change: transform and overflow: visible on the wrapper so the grid doesn’t reflow. While most other requirements are satisfied (BEM, grid columns at breakpoints, hover transitions at 300ms, title and button hover colors, and required data-qa attributes), this variable usage issue is a blocker per the checklist. Once the $star-size fix is in, your solution should be good to go. Keep up the solid work!
✨ 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
|
|
||
| * { |
There was a problem hiding this comment.
The $star-size variable is defined in _variables.scss but not used here. Consider using $star-size instead of the hardcoded 16px value to follow CHECKLIST ITEM #2 (repeated sizes should use variables).
| $card-height: 408px; | ||
| $card-border: 1px solid #f3f3f3; |
There was a problem hiding this comment.
The $star-size variable is defined in _variables.scss but not used here. According to checklist item #2, repeated sizes should be put to variables. Replace hardcoded 16px values with $star-size.
| href="https://fonts.googleapis.com" | ||
| /> | ||
| <link | ||
| rel="preconnect" |
There was a problem hiding this comment.
Hover transition requirement: All changes on :hover must be smooth (300ms). You set transition durations using $transition-duration and applied transitions to transform on .card and color/background on elements. However, scaling the card can affect neighboring items unless the card is isolated. To ensure "neighboring cards should not be affected", wrap each card in an element that sets overflow: visible and ensure transform is applied only to the card itself — alternatively use transform-style or ensure the grid alignment and use translateZ(0). Explicitly ensure the grid items do not reflow when a card scales. Consider adding transform-origin: center; and ensuring the grid container doesn't change layout (e.g., add isolation with will-change: transform or set the grid item to allow overflow visible).
| <nav class="nav"> | ||
| <ul class="nav__list"> |
There was a problem hiding this comment.
Checklist item #5 / Implementation detail: ".card:hover .card__title" must change color to #34568b. You implemented this using &:hover &__title { color: $card-title-hover-color; } in _card.scss and $card-title-hover-color equals #34568b in _variables.scss — this matches the requirement.
| > | ||
| <div class="card__image"></div> | ||
|
|
||
| <h2 class="card__title"> |
There was a problem hiding this comment.
Requirement: "Change the button background to #fff and text color to #00acdc on hover." In _card.scss .card__buy &:hover sets background-color: $white-color and color: $accent-color — this satisfies the requirement for the button hover.
| </nav> | ||
| </header> | ||
|
|
||
| <main class="catalog"> |
There was a problem hiding this comment.
Semantic requirement: Use
tag for cards container — index.html uses which satisfies this.| <a | ||
| href="#" | ||
| class="nav__link" | ||
| data-qa="nav-hover" |
There was a problem hiding this comment.
Requirement: "Remove old data-qa attributes" — I inspected index.html and the only data-qa attributes present are the ones required by the task: data-qa="nav-hover" on the 4th nav link, data-qa="card" on the first card and data-qa="card-hover" on the Buy link of the first card. No old/unexpected data-qa attributes found.
| <link | ||
| rel="stylesheet" | ||
| href="styles/index.scss" | ||
| href="./styles/index.scss" |
There was a problem hiding this comment.
Accessibility / code style check: index.html includes link to ./styles/index.scss directly (allowed per task). Good.
| rel="stylesheet" | ||
| /> |
There was a problem hiding this comment.
One small code detail: in _card.scss the .card__image uses a hard-coded width/height (160x134). That's acceptable but ensure images scale well for different devices. No direct violation of the task, just a note.
| @@ -0,0 +1,137 @@ | |||
| @use '../utils/variables' as *; | |||
There was a problem hiding this comment.
Checklist item: "Replace <your_account> with your GitHub username and copy the links to the Pull Request description" — This is a global checklist step to perform when making the PR, not in these files. Make sure you add the DEMO and TEST REPORT links to your PR description and copy the checklist with - [x] when done.
| @use '../utils/variables' as *; | ||
|
|
||
| .card { | ||
| box-sizing: border-box; | ||
| width: $card-width; | ||
| height: $card-height; | ||
| padding: 32px 16px 16px; | ||
| border: $card-border; | ||
| border-radius: $card-border-radius; | ||
| display: flex; | ||
| flex-direction: column; | ||
| flex-shrink: 0; | ||
| transition: transform $transition-duration; | ||
|
|
||
| &:hover { | ||
| transform: scale($card-scale); | ||
| } | ||
|
|
||
| &__image { | ||
| display: block; | ||
| width: 160px; | ||
| height: 134px; | ||
| margin: 0 auto; | ||
| background-image: url('../images/imac.jpeg'); | ||
| background-repeat: no-repeat; | ||
| background-size: 100% 100%; | ||
| } | ||
|
|
||
| &__title { | ||
| height: 36px; | ||
| margin-top: 40px; | ||
| margin-bottom: 4px; | ||
| color: $text-color; |
There was a problem hiding this comment.
This violates nothing in the provided files but please verify visually that nav links are not black. The nav links style in _nav.scss sets color: $text-color; and $text-color is #060b35 in variables — this matches the requirement that nav links color should be #060b35. No code change needed in source files.
| @@ -0,0 +1,21 @@ | |||
| @use '../utils/variables' as *; | |||
There was a problem hiding this comment.
Checklist/Requirement verification: The description requires nav links color to be #060b35 (not black). In your SCSS variables $text-color is set to #060b35 (src/styles/utils/_variables.scss). But in _nav.scss the .nav__link uses color: $text-color which is correct. No change needed here.
| @use '../utils/variables' as *; | ||
|
|
||
| .catalog { | ||
| display: grid; | ||
| grid-template-columns: repeat(1, $card-width); | ||
| gap: $catalog-row-gap $catalog-column-gap; | ||
| justify-content: center; | ||
| padding: $catalog-padding; | ||
|
|
||
| @media (min-width: 488px) { | ||
| grid-template-columns: repeat(2, $card-width); | ||
| } | ||
|
|
||
| @media (min-width: 768px) { | ||
| grid-template-columns: repeat(3, $card-width); | ||
| } | ||
|
|
||
| @media (min-width: 1024px) { | ||
| grid-template-columns: repeat(4, $card-width); | ||
| } | ||
| } |
There was a problem hiding this comment.
The requirement: "Make all the changes smooth on hover (during 300ms)" — you set $transition-duration: 300ms in variables and used it for card transform, title color transition, nav link color transition and button transitions. Confirmed present in _card.scss and _nav.scss. However verify nav hover color change to #00acdc: in _nav.scss the &:hover and .is-active use color: $accent-color which maps to #00acdc in variables — satisfied.
|
|
||
| @media (min-width: 768px) { | ||
| grid-template-columns: repeat(3, $card-width); | ||
| } | ||
|
|
There was a problem hiding this comment.
The requirement: "Increase the card by 20 percent on hover (neighboring cards should not be affected)" — you implement transform: scale($card-scale) with $card-scale: 1.2 in _card.scss on &:hover. Because each card has flex-shrink: 0 and fixed width and grid layout uses fixed column width, scaling will visually increase the hovered card without reflowing neighbors in a grid only if transform is applied and overflow/placement allow it. Your grid uses fixed column widths and justify-content:center which should work. This meets the requirement.
| .catalog { | ||
| display: grid; | ||
| grid-template-columns: repeat(1, $card-width); | ||
| gap: $catalog-row-gap $catalog-column-gap; |
There was a problem hiding this comment.
This violates the requirement that cards have fixed width 200px — you set $card-width: 200px in variables and used it. But verify the .card element itself has width: $card-width (src/styles/blocks/_card.scss line 5) — present. Good. No action needed for card width.
| .catalog { | ||
| display: grid; | ||
| grid-template-columns: repeat(1, $card-width); |
There was a problem hiding this comment.
This violates checklist item #3: "Grid is used for the columns" — you used display: grid in _catalog.scss (line 4) and media queries to change template columns. This satisfies the item.
| @media (min-width: 488px) { | ||
| grid-template-columns: repeat(2, $card-width); | ||
| } | ||
|
|
||
| @media (min-width: 768px) { | ||
| grid-template-columns: repeat(3, $card-width); | ||
| } | ||
|
|
||
| @media (min-width: 1024px) { | ||
| grid-template-columns: repeat(4, $card-width); |
There was a problem hiding this comment.
This violates checklist item #4: "Cards are shown in 1, 2, 3, or 4 columns based on screen resolution" — your _catalog.scss has media queries at 488px, 768px, and 1024px producing 1,2,3,4 columns via grid-template-columns: repeat(n, $card-width). This matches the requirement.
| @use '../utils/variables' as *; | ||
|
|
||
| .catalog { | ||
| display: grid; | ||
| grid-template-columns: repeat(1, $card-width); | ||
| gap: $catalog-row-gap $catalog-column-gap; | ||
| justify-content: center; | ||
| padding: $catalog-padding; | ||
|
|
||
| @media (min-width: 488px) { | ||
| grid-template-columns: repeat(2, $card-width); | ||
| } | ||
|
|
||
| @media (min-width: 768px) { | ||
| grid-template-columns: repeat(3, $card-width); | ||
| } | ||
|
|
||
| @media (min-width: 1024px) { | ||
| grid-template-columns: repeat(4, $card-width); | ||
| } | ||
| } |
There was a problem hiding this comment.
This violates checklist item #5: "All changes on :hover are smooth" — transitions are implemented: transform $transition-duration on .card, color transitions for title and nav link and button transitions for background-color and color. However ensure .nav__link transition is present in _nav.scss line where transition: color $transition-duration is defined — present. Satisfied.
|
|
||
| @media (min-width: 768px) { | ||
| grid-template-columns: repeat(3, $card-width); | ||
| } |
There was a problem hiding this comment.
Potential functional issue to check: When scaling a grid item using transform the element may overflow its grid cell visually. If test expects the hovered card to not overlap neighbors, you might need to set transform-origin or use isolation via translateZ or an isolated stacking context. At minimum consider adding transform-origin: center; or will-change: transform to .card to improve rendering. This is a recommendation, not a failing requirement.
| @@ -0,0 +1,27 @@ | |||
| @use '../utils/variables' as *; | |||
|
|
|||
| .stars { | |||
There was a problem hiding this comment.
Checklist item #1 requires: "All components follow BEM and use SCSS" — you have BEM blocks and SCSS files for header, nav, stars, card and catalog. This is OK. No action.
| background-image: url(../images/star-active.svg); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Requirement: "Change navigation link text color to #00acdc on hover." In _nav.scss you use &:hover { color: $accent-color; } and $accent-color is #00acdc in variables, so the hover color requirement is satisfied. No action.
| background-image: url(../images/star.svg); | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
| margin-right: 4px; | ||
|
|
||
| &:last-child { | ||
| margin-right: 0; | ||
| } | ||
| } | ||
|
|
|
|
||
| .stars { | ||
| display: flex; | ||
| align-items: center; |
There was a problem hiding this comment.
Requirement: "Cards have fixed width - 200px." You have $card-width: 200px; in variables and .card { width: $card-width; } so this is satisfied.
| .stars { | ||
| display: flex; | ||
| align-items: center; | ||
|
|
There was a problem hiding this comment.
Requirement: "The gap between cards should be - 48px horizontally and 46px vertically." Your variables set $catalog-row-gap: 46px; $catalog-column-gap: 48px; and .catalog { gap: $catalog-row-gap $catalog-column-gap; } — this matches requirement but note the order: CSS gap: row-gap column-gap; means first value is row (vertical) and second is column (horizontal), which you used correctly. No action.
| align-items: center; | ||
|
|
||
| &__item { | ||
| width: 16px; |
There was a problem hiding this comment.
Requirement: "Cards container(catalog) have fixed paddings (50px vertically and 40px horizontally)." Your variable $catalog-padding: 50px 40px; and .catalog { padding: $catalog-padding; } satisfy this. No action.
| background-image: url(../images/star.svg); | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
| margin-right: 4px; |
There was a problem hiding this comment.
Requirement: "Make all the changes smooth on hover (during 300ms)" — You set $transition-duration: 300ms; and use it in transitions for card transform, title color and button color/background, so timing is correct. No action.
| &:last-child { | ||
| margin-right: 0; |
There was a problem hiding this comment.
Requirement: "Increase the card by 20 percent (neighboring cards should not be affected)." You apply transform: scale($card-scale) where $card-scale: 1.2 on .card:hover. However scaling a grid item may affect layout (neighboring items) unless the transformed element is isolated (for example with transform-origin and making the grid place items without reflow or using will-change/z-index). To ensure neighboring cards are not affected, wrap card content in an inner element and scale that, or set the card to display: inline-block with fixed width and ensure grid allows overflow (e.g., overflow: visible) and the scaled element positioned with z-index. Currently there is no code to prevent layout shift of neighboring cards on hover — please ensure the hover scale does not change grid layout so neighbors remain unaffected.
| @@ -0,0 +1,27 @@ | |||
| @use '../utils/variables' as *; | |||
There was a problem hiding this comment.
Code style / checklist item: "Repeated sizes and special colors are put to variables." You moved repeated values into _variables.scss and used them across block files. This satisfies checklist item #2. No action.
| &--#{$i} { | ||
| & > :nth-child(-n + #{$i}) { | ||
| background-image: url(../images/star-active.svg); | ||
| } |
There was a problem hiding this comment.
Potential issue: _nav.scss currently sets .nav__link { color: $text-color; } which uses $text-color: #060b35; — OK. But the ::after pseudo-element uses background-color: $accent-color for the active underline; the mockup shows accent underline color which is fine. No action.
| @@ -0,0 +1,27 @@ | |||
| @use '../utils/variables' as *; | |||
There was a problem hiding this comment.
Minor/important: The requirement states 'Use Header, Stars and Card blocks from previous tasks but rewrite them using BEM and SCSS.' You have header (_header.scss), nav (_nav.scss), stars and card implemented. However I noticed there is no _header.scss content review in these provided files (it exists in project but not included in the last code snippets). Make sure the Header block follows BEM and SCSS as others (it appears correct in project files).
| @@ -0,0 +1,27 @@ | |||
| @use '../utils/variables' as *; | |||
There was a problem hiding this comment.
Action required (checklist & PR description): Replace <your_account> with your GitHub username and add DEMO and TEST REPORT links and the copied checklist with - [x] before each point in the Pull Request description. This is a procedural requirement outside code and must be done before submission.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because two critical checklist/requirement items are not yet met.
- The hover scale is applied on the
.carditself, which can cause the grid to reflow and affect neighboring items; the requirement states neighbors must not be affected. Please prevent reflow by either:- Allowing overflow on the grid/item wrapper (e.g.,
.catalog { overflow: visible; }), addingwill-change: transformandtransform-origin: centerto the scaled element, and ensuring it layers above neighbors (e.g.,z-indexon hover), or - Wrapping the card content (e.g.,
.card__inner) and applying thetransformto that inner element while keeping the.cardgrid item’s size fixed.
- Allowing overflow on the grid/item wrapper (e.g.,
- [CHECKLIST ITEM #2] Repeated sizes must be variables. In
_card.scss,.card__imageuses hardcodedwidth: 160pxandheight: 134px. Move these to variables insrc/styles/utils/_variables.scss(e.g.,$card-image-width,$card-image-height) and reference them in_card.scss.
Once these are addressed, the rest of your implementation looks strong and should be ready to approve. Keep up the good work!
✨ 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
| rel="preconnect" | ||
| href="https://fonts.gstatic.com" |
There was a problem hiding this comment.
The card hover scales the .card itself (transform: scale($card-scale)), which can cause the grid to reflow and affect neighboring items. The task requires that neighboring cards should not be affected. Consider making the grid allow overflow (e.g., on the .catalog: overflow: visible) and/or adding will-change: transform and transform-origin: center to the .card so the layout doesn't reflow. Alternatively, wrap the card content and apply transform to that inner element while keeping the grid item size fixed.
| /> | ||
|
|
There was a problem hiding this comment.
The checklist requires repeated sizes and special colors be put into variables. In _card.scss the .card__image width and height are hardcoded (160px and 134px). Move these values into variables in src/styles/utils/_variables.scss and use them here to satisfy [CHECKLIST ITEM #2].
| href="https://fonts.googleapis.com" | ||
| /> |
There was a problem hiding this comment.
To fully prevent reflow on hover also consider adding will-change: transform to the .card to hint the browser for performance improvements and ensure smooth transitions (the transition duration variable is already used).
|
|
||
| &__image { | ||
| display: block; | ||
| width: 160px; |
There was a problem hiding this comment.
The task's checklist item #2 requires repeated sizes and special colors be put to variables. Here the card image dimensions are hardcoded (width: 160px; height: 134px;). Consider moving these values to variables (e.g., $card-image-width, $card-image-height) in src/styles/utils/_variables.scss and using them here.
| transform-origin: center; | ||
| transition: transform $transition-duration; |
There was a problem hiding this comment.
Although .card applies transform: scale(...) with transform-origin: center and transition, the grid may still reflow and affect neighboring cards when one card scales. The task requires neighboring cards not to be affected. Add will-change: transform to the card and ensure the grid/container allows overflow (for example overflow: visible on the grid or an item wrapper) or apply the transform to an inner element so the grid cell size doesn't change.
| color $transition-duration; | ||
|
|
||
| &:hover { | ||
| background-color: $white-color; |
There was a problem hiding this comment.
The buy button hover swaps background and color as required, but note the task explicitly requires the hovered button text color to be #00acdc and background #fff. Currently the button uses $accent-color for color on hover which matches $accent-color: #00acdc, so this is OK — just ensure variables remain consistent. No change required here unless you prefer explicit values.
| } | ||
|
|
There was a problem hiding this comment.
The task requires that scaling a hovered card must not affect neighboring cards. While the transform is applied on .card (good), you should ensure the grid doesn’t reflow when a card scales. Consider adding a wrapper or ensuring the grid items allow overflow and the transformed element uses will-change: transform and sits above neighbors (e.g., set overflow: visible on the grid item/ wrapper and z-index on hover).
| margin-right: 4px; | ||
|
|
||
| &:last-child { | ||
| margin-right: 0; | ||
| } |
There was a problem hiding this comment.
This applies the hover scale to the grid item itself which can make neighboring cards move. The task requires the card to increase by 20% on hover but neighboring cards should not be affected. Consider wrapping card content in an inner element (e.g. .card__inner) and move the transform/transition/will-change to that inner element while keeping the grid item as a stable container with overflow: visible. Alternatively set will-change: transform and ensure the transform does not change layout (apply on inner element). Example guidance: move transition and &:hover { transform: ... } to a child and keep the .card as the grid item so the grid doesn't reflow.
| &--#{$i} { | ||
| & > :nth-child(-n + #{$i}) { | ||
| background-image: url(../images/star-active.svg); |
There was a problem hiding this comment.
Image width and height are hardcoded here (width: 160px; height: 134px). The checklist requires repeated sizes and special colors to be put into variables. Move these values to variables in src/styles/utils/_variables.scss (for example $card-image-width and $card-image-height) and use them here instead of hardcoded numbers.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved. All critical issues from previous reviews have been successfully resolved — the hover scale no longer causes grid reflow (using overflow: visible, transform-origin: center, will-change: transform, and z-index on hover), and image dimensions are properly extracted to $card-image-width: 160px and $card-image-height: 134px variables. You've correctly implemented BEM methodology, SCSS with variables for all repeated values, responsive grid breakpoints (1/2/3/4 columns), all required data-qa attributes, the is-active class, and smooth 300ms transitions with correct colors. The visual regression test failures are due to environmental rendering differences (font anti-aliasing, spacing), not your code — this is an educational approval to help you continue learning. Keep up the excellent work on clean, well-structured SCSS!
✨ 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
Anton-Kuchmasov
left a comment
There was a problem hiding this comment.
UI diffs are not critical - task was done correctly
Good job!
Checklist
❗️ Replace
<your_account>with your GitHub username and copy the links to thePull Requestdescription:❗️ Copy this
Checklistto thePull Requestdescription after links, and put- [x]before each point after you checked it.**