Solution#6237
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I'm approving your solution with this educational note. You've done an excellent job implementing the core requirements: proper BEM structure across all components, correct data-qa attributes placement (nav-hover, card, card-hover, is-active), functional responsive grid layout with the right breakpoints, and consistent SCSS usage. However, there are a few consistency issues to address: 1) The transition-duration property should use the shorthand transition syntax for cleaner code; 2) Gap values in index.html (46px, 48px) and star sizes in stars.scss (13.6px, 6.4px) should be added to variables.scss and imported instead of being hardcoded; 3) The :root block with --blue-color in header.scss belongs in index.scss since :root is a global selector; 4) Consider using transform: scale(1.2) translateZ(0) on card hover to prevent potential overlap with neighboring cards. These are minor improvements that will make your code more maintainable and consistent with the checklist requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.gstatic.com" | ||
| crossorigin="anonymous" |
There was a problem hiding this comment.
Gap values are hardcoded (46px 48px) instead of using variables. According to checklist item #2, repeated sizes and special colors must be put to variables. The variables.scss file should define these gap values.
| > | ||
| Photo | ||
| </a> | ||
| </li> |
There was a problem hiding this comment.
The card scales on hover but CSS transform doesn't remove the element from document flow. This means the scaled card will visually overlap with neighboring cards. The requirement states 'neighboring cards should not be affected' - consider using transform with proper positioning to prevent overlap.
| margin: 0 auto 40px; | ||
| } | ||
|
|
||
| &__title { | ||
| margin: 0 0 4px; | ||
| font-size: 12px; | ||
| line-height: 18px; | ||
| font-weight: 500; | ||
| color: $main-color; |
There was a problem hiding this comment.
The .header__logo img selector is not BEM-compliant. According to BEM methodology, nested element selectors should be avoided. Consider using a proper .header__logo__image class or keeping the styling in .header__logo with an img selector that's scoped properly.
| width: $card-width; | ||
| height: $card-height; | ||
| border: 1px solid $border-color; |
There was a problem hiding this comment.
The :root block defines CSS variable --blue-color: #00acdc while variables.scss already has $blue-color: #00acdc. This creates inconsistency - the checklist requires repeated colors to be put to variables. The header should use the SCSS variable $blue-color imported from variables.scss instead of defining a separate CSS variable.
| text-transform: uppercase; | ||
| &:hover { | ||
| background-color: $white-color; | ||
| color: $blue-color; | ||
| height: 38.5px; | ||
| transform: scale(1.2); | ||
| transition-duration: $transition-duration; |
There was a problem hiding this comment.
The hover state has transform: scale(1.2) which conflicts with the parent's .card:hover that also applies transform: scale(1.2). The button transform should be removed - when hovering the card, the card itself scales, and the button hover should only handle its own color/background changes.
|
|
||
| :root { | ||
| --blue-color: #00acdc; | ||
| } |
There was a problem hiding this comment.
The :root block with CSS custom variable --blue-color is placed in header.scss. This should be moved to index.scss or a global stylesheet since :root is a global selector, not component-specific.
| justify-content: center; | ||
| gap: 46px 48px; | ||
| padding: 50px 40px; | ||
| } |
There was a problem hiding this comment.
The .header__logo class uses display: flex and position: relative, but since .header__logo is applied to an <img> element (a replaced element), display: flex has no effect. These properties could be removed or the logo image should be wrapped in a container.
| display: flex; | ||
| gap: 6.4px; |
There was a problem hiding this comment.
The star gap value (6.4px) is hardcoded. According to checklist item #2, repeated sizes must be put to variables. Add a $star-gap variable to variables.scss and use it here.
| &__star { | ||
| width: 13.6px; | ||
| height: 13.6px; |
There was a problem hiding this comment.
Star dimensions (13.6px) are hardcoded in width and height. These repeated sizes should be defined as variables in variables.scss.
| .stars { | ||
| display: flex; | ||
| gap: 6.4px; | ||
|
|
||
| &__star { | ||
| width: 13.6px; | ||
| height: 13.6px; | ||
| background-image: url(../images/star.svg); | ||
| background-repeat: no-repeat; | ||
| background-position: center; |
There was a problem hiding this comment.
Variables.scss is well-structured and contains the core variables for colors, card dimensions, and transitions. Consider adding the missing variables for gap values ($catalog-gap-x: 48px, $catalog-gap-y: 46px) and star sizes ($star-size: 13.6px, $star-gap: 6.4px) used elsewhere.
No description provided.