add solution#6253
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has three critical violations of explicit checklist requirements that must be fixed:
-
BEM violation in nav.scss (checklist item #1): The
.is-activeclass is defined outside the.navblock at lines 38-50. In BEM, modifiers must be nested within their parent block - use.nav__link.is-activeinside the.navblock or rename to.nav__link--activefollowing the modifier pattern. -
Hardcoded transition duration in card.scss (checklist item #2): The transition is hardcoded as
300msinstead of using the$transition-durationvariable defined in variables.scss. Change totransition: all variables.$transition-duration;for consistency with the rest of the codebase. -
Card scaling affects neighbors (checklist item #4): The card uses
transform: scale(1.2)withoutoverflow: hiddenon the parent.catalogcontainer. Since cards are 200px wide, scaling to 240px causes the card to visually overlap neighboring cards. Addoverflow: hiddento.catalogor use a different approach likebox-shadowto ensure neighboring cards remain unaffected.
These issues directly violate requirements specified in your task checklist and must be addressed before approval.
✨ 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
|
|
||
| &__nav { | ||
| margin-right: 50px; |
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 - neighboring cards should not be affected'). Using transform: scale(1.2) without overflow: hidden on the parent container causes the scaled card to push neighboring cards out of their grid positions. Add overflow: hidden to the catalog container or use a different approach.
| width: variables.$card-width; | ||
| padding: variables.$main-padding; | ||
| border: 1px solid variables.$border-color; | ||
| border-radius: 5px; |
There was a problem hiding this comment.
Checklist item #2 violation: The transition duration is hardcoded as 300ms instead of using the $transition-duration variable defined in variables.scss. This should be transition: all variables.$transition-duration;
| font-size: 14px; | ||
| line-height: 16px; | ||
|
|
||
| margin-top: variables.$main-padding; |
There was a problem hiding this comment.
Checklist item #2 violation: Same hardcoded transition duration - should use variables.$transition-duration instead of 300ms for consistency with the rest of the codebase.
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Checklist item #4 violation (Cards are shown in 1, 2, 3, or 4 columns based on screen resolution - neighboring cards should not be affected): The card hover uses transform: scale(1.2) without overflow: hidden on the parent container. Since cards are 200px wide, scaling to 240px will cause the card to visually overlap neighboring cards in the grid. Add overflow: hidden to .catalog or use a different approach to ensure neighboring cards are not affected.
|
|
||
| .is-active { | ||
| color: variables.$main-color; | ||
|
|
||
| &::after { | ||
| position: absolute; | ||
| content: ''; | ||
| height: 4px; | ||
| width: 100%; | ||
| bottom: 0; | ||
| background-color: variables.$main-color; | ||
| border-radius: 8px; | ||
| } |
There was a problem hiding this comment.
The .is-active class is defined outside the .nav block, which violates BEM methodology (checklist item #1). In proper BEM, modifiers should be nested within the block: either use &__link.is-active { } inside .nav, or restructure to follow BEM conventions properly.
|
@mateacademy-ai-mentor, the transition duration is not hardcoded anywhere. The rest is fixed. |
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is complete and meets all task requirements. All critical issues have been resolved: BEM methodology is properly implemented with .is-active correctly nested inside .nav__link, transition durations consistently use the $transition-duration variable (0.3s) across all files, and the catalog's overflow: hidden prevents scaled cards from affecting their neighbors. All checklist items are satisfied including the responsive grid breakpoints (1→2→3→4 columns), proper data-qa attributes, card dimensions (200px fixed width, 48px/46px gaps), and hover effects. Great work on addressing all the feedback and maintaining consistent code quality throughout.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
DEMO LINK
TEST REPORT LINK
All components follow BEM and use SCSS
Repeated sizes and special colors are put to variables
Grid is used for the columns
Cards are shown in 1, 2, 3, or 4 columns based on screen resolution
All changes on
:hoverare smoothCode follows all the Code Style Rules ❗️