complit luyot catalog#6225
Conversation
142536Def
commented
May 21, 2026
142536Def
commented
- DEMO LINK
- TEST REPORT LINK
- [x]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 :hover are smooth
- [x]Code follows all the Code Style Rules ❗️
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation requires changes before approval. Critical issue: The card's transform: scale(1.2) on :hover lacks a transition property, making the 20% scale-up instant instead of smooth 300ms as explicitly required. Add transition: transform 0.3s ease; to the .card base rule in src/styles/_card.scss. Medium issue: Cards 2-8 still contain data-qa="hover" attributes that should be removed per the 'remove old data-qa attributes' requirement. Minor issue: The .is-active class should be BEM-scoped under .nav (e.g., .nav__link.is-active) rather than standalone to follow BEM conventions. The catalog padding at 1024px (changing to 50px instead of 40px) should also be corrected per the specification. These issues prevent the code from fully meeting the task requirements.
✨ 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
|
|
||
| &:hover { | ||
| transform: scale(1.2); |
There was a problem hiding this comment.
The card's transform: scale(1.2) effect lacks an explicit transition property. The requirement states all changes on :hover should be smooth during 300ms. Currently the scale effect will be instant. Add transition: transform 0.3s ease; (or include transform in a general transition property) to the .card rule.
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
These data-qa="hover" attributes should be removed per the requirement to 'remove old data-qa attributes'. Only the first card's Buy button needs data-qa="card-hover".
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
These data-qa="hover" attributes should be removed per the requirement to 'remove old data-qa attributes'.
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
These data-qa="hover" attributes should be removed per the requirement to 'remove old data-qa attributes'.
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
These data-qa="hover" attributes should be removed per the requirement to 'remove old data-qa attributes'.
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
These data-qa="hover" attributes should be removed per the requirement to 'remove old data-qa attributes'.
| <a | ||
| href="#" | ||
| class="card__button" | ||
| data-qa="hover" |
There was a problem hiding this comment.
These data-qa="hover" attributes should be removed per the requirement to 'remove old data-qa attributes'.
| class="nav__link is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a |
There was a problem hiding this comment.
According to BEM methodology, .is-active should be scoped under a BEM block (e.g., .nav__link.is-active or nested inside .nav). Currently it's defined outside any block.
| margin-right: 0; | ||
| } | ||
|
|
||
| &--1 &__star:nth-child(-n + 1), |
There was a problem hiding this comment.
The horizontal padding changes to 50px at 1024px, but the requirement specifies cards container should have fixed 40px horizontal padding. Remove the padding override from this media query or set it to 40px.
| @import './variables'; | ||
| @import './header'; | ||
| @import './catalog'; | ||
| @import './stars'; | ||
| @import './card'; | ||
|
|
||
| body { | ||
| margin: 0; | ||
| font-family: Roboto, sans-serif; | ||
| color: #333; | ||
| } | ||
|
|
||
| * { | ||
| padding: 0; | ||
| margin: 0; | ||
| box-sizing: border-box; | ||
| } | ||
|
|
||
| *, | ||
| *::before, | ||
| *::after { | ||
| box-sizing: border-box; | ||
| } | ||
|
|
||
| /* Links */ | ||
|
|
There was a problem hiding this comment.
The card's scale transform effect (line 10-11 in _card.scss) lacks a transition property. The requirement states all changes on :hover should be smooth during 300ms. Add transition: transform 0.3s; to make the card scale-up smooth.
2pasha
left a comment
2pasha
left a comment
There was a problem hiding this comment.
lgtm 🔥
I recommend to add some transition for hover effect
3 participants
