Well Done!#6232
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has two critical blockers that must be fixed before approval. First, the 3-column responsive breakpoint uses min-width: 488px in both card.scss and index.html, which overlaps with the 2-column breakpoint at the same threshold—change the 3-column media query to use min-width: 768px to create proper non-overlapping ranges. Second, the CSS gap property applies values as row-gap column-gap (vertical then horizontal), but the current implementation in var.scss has $row-gap: 48px and $column-gap: 46px, resulting in gap: 46px 48px which reverses the required 48px horizontal and 46px vertical—swap these values so horizontal gap is 48px. Additionally, hardcoded color values #616070 and #060b35 in card.scss should be replaced with $secondary-accent and $main-accent variables for consistency.
✨ 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
|
|
||
| @media (min-width: 488px) and (max-width: 1023px) { | ||
| .catalog { | ||
| grid-template-columns: repeat(3, $card-width); | ||
| } |
There was a problem hiding this comment.
The 3-column breakpoint overlaps with the 2-column breakpoint. Since both use min-width: 488px, at 768px screen width, both media queries match and the 2-column rule (being later) takes precedence. Change min-width: 488px to min-width: 768px for the 3-column rule to create proper progression: 1 column → 2 columns (488px) → 3 columns (768px) → 4 columns (1024px).
| margin-bottom: 16px; | ||
| } | ||
|
|
||
| &__text { |
There was a problem hiding this comment.
Color #616070 is hardcoded here but $secondary-accent variable is already defined with this value. Use the variable for consistency.
| margin: 0; | ||
| } | ||
|
|
||
| &__suma { |
There was a problem hiding this comment.
Color #060b35 is hardcoded here but $main-accent variable is already defined with this value. Use the variable for consistency.
| display: grid; | ||
| justify-content: center; | ||
| align-items: start; | ||
| gap: $column-gap $row-gap; |
There was a problem hiding this comment.
The CSS gap property syntax is gap: row-gap column-gap. Currently it's gap: $column-gap $row-gap which reverses the values. The horizontal gap (48px) should come first, then vertical gap (46px). Swap to gap: $row-gap $column-gap.
|
|
||
| <section class="card"> | ||
| <img | ||
| src="./images/imac.jpeg" | ||
| alt="monitor" | ||
| class="card__photo" | ||
| /> | ||
| <div class="card__main"> | ||
| <h2 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h2> | ||
| <p class="card__code">Product code: 195434</p> | ||
| </div> | ||
|
|
||
| <div class="card__container"> | ||
| <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> | ||
| <p class="card__reviews">Reviews: 5</p> |
There was a problem hiding this comment.
These media query ranges overlap. The second query (488px-767px for 2 columns) is completely overridden by the third query (488px-1023px for 3 columns) at widths >=768px. Consider restructuring: 1 column (max-width: 487px), 2 columns (min-width: 488px), 3 columns (min-width: 768px), 4 columns (min-width: 1024px) - removing the max-width conditions to create non-overlapping ranges.
| data-qa="card" | ||
| > | ||
| <img | ||
| src="./images/imac.jpeg" |
There was a problem hiding this comment.
This color value is hardcoded as '#060b35' but the variable $main-accent is defined in var.scss and should be used instead for consistency.
| @import '/src/styles/header'; | ||
| @import '/src/styles/card'; | ||
| @import '/src/styles/stars'; | ||
|
|
There was a problem hiding this comment.
The gap values are swapped. According to requirements, horizontal gap should be 48px and vertical gap should be 46px, but $row-gap is 48px and $column-gap is 46px. In CSS grid, the gap property uses 'gap: row-gap column-gap', so $column-gap 46px is applied horizontally and $row-gap 48px is applied vertically.
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| padding: 0 50px; |
There was a problem hiding this comment.
The gap values are applied in reversed CSS order. The CSS gap: $column-gap $row-gap translates to row-gap column-gap (vertical then horizontal). Consider renaming variables for clarity: $row-gap for horizontal (48px) and $column-gap for vertical (46px), then apply as gap: $row-gap $column-gap.
| .stars { | ||
| display: flex; | ||
|
|
||
| &__star { | ||
| background-image: url('../images/star.svg'); | ||
| background-position: center; |
There was a problem hiding this comment.
The CSS gap property syntax is 'row-gap column-gap', but the current gap: $column-gap $row-gap applies 46px horizontally and 48px vertically (reversed). The requirement specifies 48px horizontal and 46px vertical. Either swap the variable names or use gap: $row-gap $column-gap.
| $font-main: Roboto, sans-serif; | ||
| $main-accent: #060b35; | ||
| $secondary-accent: #616070; | ||
| $blue-accent: #00acdc; | ||
| $white-accent: #fff; | ||
| $bg-fon: #f3f3f3; |
There was a problem hiding this comment.
The CSS gap property applies values as row-gap column-gap. Currently gap: $column-gap $row-gap outputs gap: 46px 48px, which means 46px vertical and 48px horizontal. This reverses the required values. Swap to gap: $row-gap $column-gap to get 48px horizontal and 46px vertical as specified.
3 participants
No description provided.