firstCommit#6282
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a couple of critical requirements not met, even though most of the catalog implementation is solid. First, index.html currently has a <main class="catalog"> that contains another <main class="catalog__cards">; nesting main elements violates semantic HTML and breaks the explicit requirement to use a single <main> tag for the cards container (this maps to [CHECKLIST ITEM #6]). Please change the outer wrapper (around the whole page) to a non-main element such as div or section, and keep only one <main> for the cards/catalog cards container.
Second, your grid layout uses grid-template-columns: repeat(var(--column-count), 1fr); together with .card { width: 200px; } and, in some places, a fixed container width like .catalog__cards { width: 1024px; }. This combination means the columns are flexible fractions of the container and the container can overflow on small screens, which conflicts with the requirements that cards have fixed width 200px and that they appear in 1/2/3/4 columns at the specified breakpoints ([CHECKLIST ITEM #4]). Please adjust the grid so each column aligns with the 200px card width (for example, using fixed-width columns based on 200px) and make the catalog container width responsive (e.g., width: 100% plus max-width) so small screens get a proper single-column layout. Finally, remove the duplicate <link rel="stylesheet" ... index.scss> in index.html so the stylesheet is only included once, avoiding redundant loading. Once these structural and layout fixes are in place, the rest of your BEM/SCSS setup, hover behavior, and data-qa attributes will fully satisfy the task; you’re very close, and these changes will bring the solution over the finish line.
✨ 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
| <main class="catalog"> | ||
| <header class="catalog__header header"> | ||
| <a href="#"> | ||
| <img | ||
| class="header__logo-link" | ||
| src="./images/logo.png" | ||
| alt="MOYO logo" | ||
| /> | ||
| </a> | ||
|
|
||
| <nav class="header__nav menu"> | ||
| <ul class="menu__list"> | ||
| <li class="menu__item"> | ||
| <a | ||
| class="menu__link is-active" | ||
| href="#" | ||
| > | ||
| apple | ||
| </a> | ||
| </li> | ||
| <li class="menu__item"> | ||
| <a | ||
| class="menu__link" | ||
| href="#" | ||
| > | ||
| samsung | ||
| </a> | ||
| </li> | ||
| <li class="menu__item"> | ||
| <a | ||
| class="menu__link" | ||
| href="#" | ||
| > | ||
| smartphones | ||
| </a> | ||
| </li> | ||
| <li class="menu__item"> | ||
| <a | ||
| class="menu__link" | ||
| href="#" | ||
| data-qa="nav-hover" | ||
| > | ||
| laptops & computers | ||
| </a> | ||
| </li> | ||
| <li class="menu__item"> | ||
| <a | ||
| class="menu__link" | ||
| href="#" | ||
| > | ||
| gadgets | ||
| </a> | ||
| </li> | ||
| <li class="menu__item"> | ||
| <a | ||
| class="menu__link" | ||
| href="#" | ||
| > | ||
| tablets | ||
| </a> | ||
| </li> | ||
| <li class="menu__item"> | ||
| <a | ||
| class="menu__link" | ||
| href="#" | ||
| > | ||
| photo | ||
| </a> | ||
| </li> | ||
| <li class="menu__item"> | ||
| <a | ||
| class="menu__link" | ||
| href="#" | ||
| > | ||
| video | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| </nav> | ||
| </header> | ||
|
|
||
| <main class="catalog__cards"> | ||
| <div | ||
| class="card" | ||
| data-qa="card" | ||
| > | ||
| <div class="card__image"></div> | ||
|
|
||
| <div class="card__content"> | ||
| <h2 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h2> | ||
|
|
||
| <p class="card__code"> | ||
| Product code: | ||
| <span class="card__code-value">195434</span> | ||
| </p> | ||
|
|
||
| <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> | ||
| <span class="card__container-reviews">Reviews: 5</span> | ||
| </div> | ||
| <div class="card__price"> | ||
| <span class="card__price-label">Price:</span> | ||
| <span class="card__price-value">$2,199</span> | ||
| </div> | ||
|
|
||
| <a | ||
| class="card__button" | ||
| href="#" | ||
| data-qa="card-hover" | ||
| > | ||
| buy | ||
| </a> | ||
| </div> | ||
| </div> | ||
| <div class="card"> | ||
| <div class="card__image"></div> | ||
|
|
||
| <div class="card__content"> | ||
| <h2 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h2> | ||
|
|
||
| <p class="card__code"> | ||
| Product code: | ||
| <span class="card__code-value">195434</span> | ||
| </p> | ||
|
|
||
| <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> | ||
| <span class="card__container-reviews">Reviews: 5</span> | ||
| </div> | ||
| <div class="card__price"> | ||
| <span class="card__price-label">Price:</span> | ||
| <span class="card__price-value">$2,199</span> | ||
| </div> | ||
|
|
||
| <a | ||
| class="card__button" | ||
| href="#" | ||
| > | ||
| buy | ||
| </a> | ||
| </div> | ||
| </div> | ||
| <div class="card"> | ||
| <div class="card__image"></div> | ||
|
|
||
| <div class="card__content"> | ||
| <h2 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h2> | ||
|
|
||
| <p class="card__code"> | ||
| Product code: | ||
| <span class="card__code-value">195434</span> | ||
| </p> | ||
|
|
||
| <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> | ||
| <span class="card__container-reviews">Reviews: 5</span> | ||
| </div> | ||
| <div class="card__price"> | ||
| <span class="card__price-label">Price:</span> | ||
| <span class="card__price-value">$2,199</span> | ||
| </div> | ||
|
|
||
| <a | ||
| class="card__button" | ||
| href="#" | ||
| > | ||
| buy | ||
| </a> | ||
| </div> | ||
| </div> | ||
| <div class="card"> | ||
| <div class="card__image"></div> | ||
|
|
||
| <div class="card__content"> | ||
| <h2 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h2> | ||
|
|
||
| <p class="card__code"> | ||
| Product code: | ||
| <span class="card__code-value">195434</span> | ||
| </p> | ||
|
|
||
| <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> | ||
| <span class="card__container-reviews">Reviews: 5</span> | ||
| </div> | ||
| <div class="card__price"> | ||
| <span class="card__price-label">Price:</span> | ||
| <span class="card__price-value">$2,199</span> | ||
| </div> | ||
|
|
||
| <a | ||
| class="card__button" | ||
| href="#" | ||
| > | ||
| buy | ||
| </a> | ||
| </div> | ||
| </div> | ||
| <div class="card"> | ||
| <div class="card__image"></div> | ||
|
|
||
| <div class="card__content"> | ||
| <h2 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h2> | ||
|
|
||
| <p class="card__code"> | ||
| Product code: | ||
| <span class="card__code-value">195434</span> | ||
| </p> | ||
|
|
||
| <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> | ||
| <span class="card__container-reviews">Reviews: 5</span> | ||
| </div> | ||
| <div class="card__price"> | ||
| <span class="card__price-label">Price:</span> | ||
| <span class="card__price-value">$2,199</span> | ||
| </div> | ||
|
|
||
| <a | ||
| class="card__button" | ||
| href="#" | ||
| > | ||
| buy | ||
| </a> | ||
| </div> | ||
| </div> | ||
| <div class="card"> | ||
| <div class="card__image"></div> | ||
|
|
||
| <div class="card__content"> | ||
| <h2 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h2> | ||
|
|
||
| <p class="card__code"> | ||
| Product code: | ||
| <span class="card__code-value">195434</span> | ||
| </p> | ||
|
|
||
| <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> | ||
| <span class="card__container-reviews">Reviews: 5</span> | ||
| </div> | ||
| <div class="card__price"> | ||
| <span class="card__price-label">Price:</span> | ||
| <span class="card__price-value">$2,199</span> | ||
| </div> | ||
|
|
||
| <a | ||
| class="card__button" | ||
| href="#" | ||
| > | ||
| buy | ||
| </a> | ||
| </div> | ||
| </div> | ||
| <div class="card"> | ||
| <div class="card__image"></div> | ||
|
|
||
| <div class="card__content"> | ||
| <h2 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h2> | ||
|
|
||
| <p class="card__code"> | ||
| Product code: | ||
| <span class="card__code-value">195434</span> | ||
| </p> | ||
|
|
||
| <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> | ||
| <span class="card__container-reviews">Reviews: 5</span> | ||
| </div> | ||
| <div class="card__price"> | ||
| <span class="card__price-label">Price:</span> | ||
| <span class="card__price-value">$2,199</span> | ||
| </div> | ||
|
|
||
| <a | ||
| class="card__button" | ||
| href="#" | ||
| > | ||
| buy | ||
| </a> | ||
| </div> | ||
| </div> | ||
| <div class="card"> | ||
| <div class="card__image"></div> | ||
|
|
||
| <div class="card__content"> | ||
| <h2 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h2> | ||
|
|
||
| <p class="card__code"> | ||
| Product code: | ||
| <span class="card__code-value">195434</span> | ||
| </p> | ||
|
|
||
| <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> | ||
| <span class="card__container-reviews">Reviews: 5</span> | ||
| </div> | ||
| <div class="card__price"> | ||
| <span class="card__price-label">Price:</span> | ||
| <span class="card__price-value">$2,199</span> | ||
| </div> | ||
|
|
||
| <a | ||
| class="card__button" | ||
| href="#" | ||
| > | ||
| buy | ||
| </a> | ||
| </div> | ||
| </div> | ||
| </main> | ||
| </main> |
There was a problem hiding this comment.
You currently have <main class="catalog"> wrapping the whole page and then another <main class="catalog__cards"> inside it. This conflicts with the requirement to "use <main> tag for cards container" and with semantic HTML (there should be a single main landmark, not a nested one). Consider changing the outer wrapper to a div or section and keep only catalog__cards as the <main> element.
| &:hover { | ||
| transform: scale(1.2); | ||
| } | ||
|
|
||
| &__container { | ||
| display: flex; | ||
| justify-content: space-between; | ||
|
|
||
| width: calc(100% - 32px); | ||
| height: 16px; | ||
| margin-bottom: 24px; | ||
| margin-left: 16px; | ||
|
|
||
| &-reviews { | ||
| width: 53px; | ||
| height: 16px; | ||
| padding-top: 2px; | ||
|
|
||
| font-size: 10px; | ||
| line-height: 14px; | ||
| color: $main-text-color; | ||
| text-align: right; | ||
| } | ||
| } | ||
|
|
||
| &__image { | ||
| width: 160px; | ||
| height: 134px; | ||
| margin: 32px 19px 40px; | ||
|
|
||
| background-image: url(../images/imac.jpeg); | ||
| background-repeat: no-repeat; |
There was a problem hiding this comment.
There are two <link rel="stylesheet" ... index.scss"> tags (lines 21–24 and 50–53). This can cause styles to load twice; remove one so you only include index.scss once.
| .card { | ||
| cursor: pointer; | ||
|
|
||
| box-sizing: border-box; | ||
| width: 200px; | ||
| height: 408px; |
There was a problem hiding this comment.
Using grid-template-columns: repeat(var(--column-count), 1fr) together with .card { width: 200px; } means column width is flexible while card width is fixed, which can misalign columns and break the "cards have fixed width - 200px" requirement. Consider defining columns so they match the 200px card width explicitly while still enforcing 1/2/3/4 columns at the specified breakpoints.
| display: grid; | ||
| grid-gap: 46px 48px; | ||
| grid-template-columns: repeat(var(--column-count), 1fr); |
There was a problem hiding this comment.
The grid uses grid-template-columns: repeat(var(--column-count), 1fr); even though cards have width: 200px. This can violate the requirement that cards have a fixed width of 200px and that 1/2/3/4 columns are shown at given breakpoints; consider using fixed column widths (e.g., 200px) or another approach that ensures columns match the card width.
| justify-content: space-between; | ||
|
|
||
| padding: 0 50px; |
There was a problem hiding this comment.
Using grid-template-columns: repeat(var(--column-count), 1fr); together with .card { width: 200px; } means column width is controlled by the grid, not truly fixed at 200px; to satisfy the requirement "cards have fixed width - 200px", consider using fixed-width columns (e.g., repeat(var(--column-count), 200px)) or otherwise ensuring cards are not stretched by the grid.
| margin: 0; | ||
| padding: 0; | ||
| list-style: none; |
There was a problem hiding this comment.
The grid uses grid-template-columns: repeat(var(--column-count), 1fr); while .card has width: 200px;. This can prevent cards from truly having fixed 200px width and may conflict with the requirement that cards be fixed-width. Consider defining grid columns to match the card width explicitly while preserving 1/2/3/4 columns per breakpoint.
| padding: 0; | ||
| list-style: none; | ||
| } | ||
|
|
There was a problem hiding this comment.
.catalog__cards sets width: 1024px;, which can break the responsive layout on smaller screens required by checklist item #4 (1/2/3/4 columns by resolution). Use a more flexible width (e.g., width: 100% with a max-width) so the catalog can adapt to narrow viewports without horizontal overflow.
| margin-right: 17px; | ||
|
|
||
| &__star { |
There was a problem hiding this comment.
The grid uses grid-template-columns: repeat(var(--column-count), 1fr); while .card has width: 200px;, so column widths are flexible fractions of the container, not explicitly 200px; this can break the “cards have fixed width - 200px” requirement and affect the exact 1/2/3/4-column layout. Consider defining columns to match the fixed card width.
|
|
||
| &__star { | ||
| width: 16px; | ||
| height: 16px; |
There was a problem hiding this comment.
.catalog__cards has width: 1024px;, which can cause horizontal overflow on smaller screens and interfere with the “1 column for the smaller screens” requirement; prefer a flexible width (e.g., width: 100% with a max-width) so the grid can adapt to narrow viewports.
| $header-main-color: #00acdc; | ||
| $main-text-color: #060b35; | ||
| $secondary-text-color: #616070; | ||
| $button-color: #00acdc; | ||
| $title-text-color-on-hover: #34568b; | ||
| $white-color: #fff; |
There was a problem hiding this comment.
The grid uses grid-template-columns: repeat(var(--column-count), 1fr); while .card is fixed at width: 200px; this can prevent cards from truly behaving as fixed-width and may break checklist item #4 (1/2/3/4 columns). Consider defining columns with a width matching the card (e.g., repeat(var(--column-count), 200px) or similar) so the layout strictly matches the spec.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because all core functional and checklist requirements are now met: single <main> for the cards container, correct 1/2/3/4-column grid with fixed 200px cards, required paddings and gaps, smooth hover effects, proper data-qa attributes, and BEM+SCSS structure with shared color variables. There are no remaining critical blockers that would prevent the catalog from working as specified.
The repeated Google Fonts preconnect and stylesheet links in index.html (multiple rel="preconnect" to fonts.googleapis.com / fonts.gstatic.com, and more than one Roboto stylesheet link) are non-blocking but worth cleaning up: keep a single preconnect per origin and one Roboto import to align better with [CHECKLIST ITEM #6] and keep the <head> minimal. Similarly, you’ve already extracted colors into variables.scss; as a polish step for [CHECKLIST ITEM #2], you could also move commonly repeated sizes (like nav height, recurring margins, star sizes) into variables to improve maintainability. Overall, this is a solid implementation of the catalog task, with good use of BEM, SCSS, and responsive layout; the remaining points are minor refinements rather than required changes.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| display: flex; | ||
| justify-content: space-between; | ||
|
|
||
| width: calc(100% - 32px); | ||
| height: 16px; | ||
| margin-bottom: 24px; | ||
| margin-left: 16px; | ||
|
|
||
| &-reviews { | ||
| width: 53px; | ||
| height: 16px; | ||
| padding-top: 2px; | ||
|
|
||
| font-size: 10px; | ||
| line-height: 14px; | ||
| color: $main-text-color; | ||
| text-align: right; | ||
| } | ||
| } |
There was a problem hiding this comment.
You have multiple repeated preconnect links to https://fonts.googleapis.com and https://fonts.gstatic.com. While not breaking, this goes against the spirit of clean markup and can be simplified by keeping a single preconnect per origin, as implied by checklist item #6: "Code follows all the Code Style Rules ❗️".
❗️ Copy this
Checklistto thePull Requestdescription after links, and put- [x]before each point after you checked it.:hoverare smooth