made a catalog#6286
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still violations of explicit task requirements and checklist items that are considered critical blockers for this task. The description clearly states that cards must have a fixed width of 200px, but your $card-width variable is set to 198px in variables.scss and reused in other SCSS files, which conflicts with both the requirement and the 200px grid columns. Also, [CHECKLIST ITEM #2] requires that repeated sizes and special colors be moved to variables, yet colors like #060b35, #00acdc, and #34568b are still hard-coded in index.html, card.scss, and header.scss instead of using $text-main, $accent, or a dedicated variable for the hover title color.
To fix this, update $card-width to exactly 200px in src/styles/variables.scss and ensure all card-related widths use that variable so the cards and grid columns are consistent. Then, replace all remaining literal #060b35 usages with $text-main, all #00acdc usages with $accent, and introduce a new variable in variables.scss for #34568b (for example $title-hover) and use it wherever that hover color appears. The rest of your implementation (semantic structure, grid at 1/2/3/4 columns, hover transitions, data-qa attributes, and BEM/SCSS usage) looks solid, so once these critical issues are addressed your solution should fully meet the requirements. Keep going—you’re very close to a fully 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
| data-qa="nav-hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> |
There was a problem hiding this comment.
This hard-coded color duplicates the main text color #060b35 that’s already defined as $text-main in variables.scss, which violates checklist item #2: "Repeated sizes and special colors are put to variables". Consider using the existing variable here instead of a literal.
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> |
There was a problem hiding this comment.
The hover title color #34568b is a special color used in the spec; per checklist item #2 ("Repeated sizes and special colors are put to variables"), it should be stored in a variable (similar to $accent, $text-main, etc.) and referenced via that variable instead of a raw hex value.
| <a | ||
| href="#" | ||
| class="header__nav-link" | ||
| > |
There was a problem hiding this comment.
Here you use a literal #00acdc instead of the $accent variable defined in variables.scss. This duplicates a special color and violates checklist item #2: "Repeated sizes and special colors are put to variables". Reuse $accent here.
| class="header__nav-link" | ||
| > | ||
| Smartphones | ||
| </a> |
There was a problem hiding this comment.
Same as above: this #00acdc matches the accent color and should reference the $accent variable to comply with checklist item #2 about putting repeated special colors into variables.
| margin-left: auto; | ||
| align-items: center; | ||
| justify-content: flex-end; | ||
| color: #060b35; |
There was a problem hiding this comment.
This violates checklist item #2: "Repeated sizes and special colors are put to variables". Here color: #060b35; should use the $text-main variable to avoid hardcoding the special color.
|
|
||
| a { | ||
| text-decoration: none; | ||
| } |
There was a problem hiding this comment.
This violates description requirement: "cards have fixed width - 200px". Here the card width variable is set to 198px, while your grid uses 200px. Align this variable with the required 200px width.
| margin-top: 5px; | ||
| margin-left: 0; | ||
| width: 40px; | ||
| height: 40px; |
There was a problem hiding this comment.
This violates the size requirement from the description: "cards have fixed width - 200px" and checklist item #2: "Repeated sizes and special colors are put to variables". Your grid columns are 200px, but $card-width is set to 198px, so the card itself is not 200px wide. Consider updating $card-width to 200px to match the specification.
| @import 'card'; | ||
|
|
||
| body { | ||
| font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
The task requires that "cards have fixed width - 200px", but $card-width is set to 198px here. This violates that functional requirement; consider changing this variable to exactly 200px so it matches the grid column width and the description.
|
|
||
| &__star { | ||
| margin-right: 4px; | ||
| width: 16px; |
There was a problem hiding this comment.
The task description says "cards have fixed width - 200px", but $card-width is set to 198px here. This contradicts the requirement and also mismatches the 200px grid columns. Update this variable to exactly 200px to comply.
| $text-secondary: #616070; | ||
| $accent: #00acdc; | ||
| $white: #fff; | ||
| $card-width: 198px; |
There was a problem hiding this comment.
This violates the requirement "cards have fixed width - 200px" and checklist item #2 about using variables consistently. $card-width is set to 198px, but your grid columns are 200px. Update this to 200px to match the spec and the grid.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical checklist violation regarding variable usage for special colors. In src/styles/header.scss, the active navigation link (.header__nav-link.is-active) still uses a hard-coded color: #00acdc;, which breaks [CHECKLIST ITEM #2: "Repeated sizes and special colors are put to variables"] and contradicts the earlier high-priority instruction to centralize this accent color. This is considered a blocker because the task explicitly requires all special colors (like the accent) to be managed via SCSS variables, and this is the last remaining instance that does not follow that rule.
To fix this, locate .header__nav-link.is-active in header.scss (around the line where the active state color is set) and replace color: #00acdc; with color: $accent;, using the $accent variable already defined in variables.scss. Once you make this change, your implementation will consistently use variables for repeated/special colors and will fully comply with the task description and checklist. Everything else in your solution—semantic structure, BEM usage, grid layout, card sizing, paddings/gaps, and hover behaviors—looks solid, so you’re very close to a complete and compliant submission.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| <a | ||
| href="#" | ||
| class="header__nav-link" | ||
| > |
There was a problem hiding this comment.
This violates checklist item #2: "Repeated sizes and special colors are put to variables" because the active nav link still uses a hard-coded #00acdc here instead of the $accent variable you defined in variables.scss. Consider replacing color: #00acdc; with color: $accent; to keep all special colors centralized.
| height: 14px; | ||
| font-weight: 400; | ||
| font-size: 10px; | ||
| line-height: 14px; |
There was a problem hiding this comment.
This still violates checklist item "Repeated sizes and special colors are put to variables" and the previous HIGH-priority instruction to "Replace all remaining literal #00acdc usages with $accent". Here, the accent color is hard-coded as #00acdc instead of using the $accent variable. Update this to color: $accent; to keep all special colors managed via SCSS variables.
| transition: color 300ms; | ||
|
|
||
| &.is-active { | ||
| color: #00acdc; |
There was a problem hiding this comment.
This violates checklist item "Repeated sizes and special colors are put to variables" and the previous HIGH-priority requirement: you still hard-code #00acdc for the active nav link instead of using the $accent variable defined in variables.scss. Replace this literal color with $accent so all special colors are consistently managed via SCSS variables.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved — it now aligns well with the catalog requirements, including the semantic <header> / <main> structure, BEM-based Header, Stars, and Card blocks, and the correct grid behavior with 1/2/3/4 columns at the specified breakpoints. You’ve correctly used SCSS with centralized variables in variables.scss for key colors and sizes, which satisfies the variable usage expectations like the accent and hover colors [CHECKLIST ITEM #2]. The required data-qa attributes on the 4th nav link, the first card, and the first card’s Buy button are present and old ones are removed, showing careful attention to the testing hooks. Hover interactions are implemented cleanly with 300ms transitions for cards, nav links, and buttons, and the card scaling plus title color change behaves as described without affecting neighboring cards. Overall, your structure, styling, and interactions look solid and consistent with the checklist, so this is a strong, well-rounded implementation. Great work incorporating earlier feedback and turning it into a polished solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.