Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several checklist requirements that directly affect correctness and automated tests are still not met. The responsive grid in src/styles/index.scss does not implement the explicit 1/2/3/4 column layout at 0/488/768/1024px as required; instead it uses repeat(auto-fit, $card-width), so you need to add media queries at those breakpoints and set grid-template-columns to repeat(1|2|3|4, $card-width) accordingly ([CHECKLIST ITEM #4]). Hover behavior is not fully smooth: the button transition is defined inside &:hover instead of on the base .card__button, and .card__title has no transition: color 300ms;, so you should move the transition to the base button rule and add a color transition to the title ([CHECKLIST ITEM #5]). In src/styles/variables.scss, you are missing variables for values the task treats as special/repeated: add something like $card-title-hover-color: #34568b;, $card-hover-scale: 1.2;, and $transition-duration: 300ms; and then use these instead of hardcoded values in index.scss ([CHECKLIST ITEM #2]). In src/index.html, there are extra data-qa="card" attributes on non‑first cards and extra data-qa="card-hover" on non‑first Buy buttons; only the first card and its Buy button should have these attributes, so remove all other occurrences as required. Also in index.html, the logo element should follow BEM naming (e.g. header__logo) to keep the Header block consistent with the BEM convention. Once you address these issues, your solution will align well with the checklist, and the rest of your structure, variables, and BEM usage are on the right track—nice work so far.
✨ 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
| $secondary-text-color: #616070; | ||
| $card-width: 200px; | ||
| $gap-x-horizontal: 48px; | ||
| $gap-y-vertical: 46px; |
There was a problem hiding this comment.
This violates checklist item #2: "Repeated sizes and special colors are put to variables" — the file is missing variables for the card title hover color (#34568b), the hover scale (increase by 20% → scale 1.2), and the transition duration (300ms). Add e.g. $card-title-hover-color: #34568b;, $card-hover-scale: 1.2; and $transition-duration: 300ms; so these repeated values are centralized.
src/index.html
Outdated
| <img | ||
| src="images/logo.png" | ||
| alt="moyo-logo" | ||
| class="logo" |
There was a problem hiding this comment.
This element uses class logo which is not following BEM. This violates checklist item #1: 'All components follow BEM and use SCSS'. Consider renaming to a BEM class like header__logo so the Header block follows BEM.
src/index.html
Outdated
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
This data-qa="card" appears on a non-first card. The task requires: 'Remove old data-qa attributes.' Only the first card must have data-qa="card". Remove this extra attribute.
src/index.html
Outdated
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
This data-qa="card" appears on a non-first card. The task requires: 'Remove old data-qa attributes.' Only the first card must have data-qa="card". Remove this extra attribute.
src/index.html
Outdated
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
This data-qa="card" appears on a non-first card. The task requires: 'Remove old data-qa attributes.' Only the first card must have data-qa="card". Remove this extra attribute.
src/index.html
Outdated
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
This data-qa="card" appears on a non-first card. The task requires: 'Remove old data-qa attributes.' Only the first card must have data-qa="card". Remove this extra attribute.
src/index.html
Outdated
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
This data-qa="card" appears on a non-first card. The task requires: 'Remove old data-qa attributes.' Only the first card must have data-qa="card". Remove this extra attribute.
src/index.html
Outdated
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
This data-qa="card" appears on a non-first card. The task requires: 'Remove old data-qa attributes.' Only the first card must have data-qa="card". Remove this extra attribute.
src/index.html
Outdated
| </div> | ||
| <div | ||
| class="card" | ||
| data-qa="card" |
There was a problem hiding this comment.
This data-qa="card" appears on a non-first card. The task requires: 'Remove old data-qa attributes.' Only the first card must have data-qa="card". Remove this extra attribute.
src/index.html
Outdated
|
|
||
| <button | ||
| class="card__button" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
This data-qa="card-hover" appears on the Buy button of a non-first card. The task requires: 'Remove old data-qa attributes.' Only the Buy button in the first card must have data-qa="card-hover". Remove this extra attribute.
src/index.html
Outdated
|
|
||
| <button | ||
| class="card__button" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
This data-qa="card-hover" appears on the Buy button of a non-first card. The task requires: 'Remove old data-qa attributes.' Only the Buy button in the first card must have data-qa="card-hover". Remove this extra attribute.
src/index.html
Outdated
|
|
||
| <button | ||
| class="card__button" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
This data-qa="card-hover" appears on the Buy button of a non-first card. The task requires: 'Remove old data-qa attributes.' Only the Buy button in the first card must have data-qa="card-hover". Remove this extra attribute.
src/index.html
Outdated
|
|
||
| <button | ||
| class="card__button" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
This data-qa="card-hover" appears on the Buy button of a non-first card. The task requires: 'Remove old data-qa attributes.' Only the Buy button in the first card must have data-qa="card-hover". Remove this extra attribute.
src/index.html
Outdated
|
|
||
| <button | ||
| class="card__button" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
This data-qa="card-hover" appears on the Buy button of a non-first card. The task requires: 'Remove old data-qa attributes.' Only the Buy button in the first card must have data-qa="card-hover". Remove this extra attribute.
src/index.html
Outdated
|
|
||
| <button | ||
| class="card__button" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
This data-qa="card-hover" appears on the Buy button of a non-first card. The task requires: 'Remove old data-qa attributes.' Only the Buy button in the first card must have data-qa="card-hover". Remove this extra attribute.
src/index.html
Outdated
|
|
||
| <button | ||
| class="card__button" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
This data-qa="card-hover" appears on the Buy button of a non-first card. The task requires: 'Remove old data-qa attributes.' Only the Buy button in the first card must have data-qa="card-hover". Remove this extra attribute.
src/styles/index.scss
Outdated
| .main { | ||
| display: grid; | ||
| padding: 50px 40px; | ||
| grid-template-columns: repeat(auto-fit, $card-width); |
There was a problem hiding this comment.
This .main grid uses grid-template-columns: repeat(auto-fit, $card-width) which does not implement the required explicit breakpoints. This violates checklist item #4: "Cards are shown in 1, 2, 3, or 4 columns based on screen resolution." Add media queries at 488px, 768px and 1024px and set grid-template-columns: repeat(2, $card-width), repeat(3, $card-width), repeat(4, $card-width) respectively (keep 1 column as default).
src/styles/index.scss
Outdated
| transition: | ||
| background-color 300ms, | ||
| color 300ms; |
There was a problem hiding this comment.
The transition for the button is declared inside &:hover (lines below). A transition must be present on the base .card__button so the color/background change animates. This breaks the requirement: [CHECKLIST ITEM #5] "All changes on :hover are smooth." Move transition: background-color 300ms, color 300ms; to the .card__button base rule (e.g. near line 132).
|
|
||
| &__title { | ||
| margin: 40px 0 0; | ||
| color: $main-text-color; |
There was a problem hiding this comment.
The card title color changes on hover but .card__title has no transition declared, so the color change will be instant. This violates [CHECKLIST ITEM #5] "All changes on :hover are smooth." Add transition: color 300ms; to the .card__title rule.
src/styles/index.scss
Outdated
| background-color: #00acdc; | ||
| color: #fff; | ||
| font-family: Roboto, sans-serif; | ||
| font-size: 14px; | ||
| font-weight: 700; | ||
| border-radius: 4px; | ||
| border: 1px solid #00acdc; |
There was a problem hiding this comment.
Hardcoded #00acdc is used for .card__button background and border here instead of the existing $hover-color variable. Repeated/special colors should be put to variables per [CHECKLIST ITEM #2] "Repeated sizes and special colors are put to variables." Replace these hardcoded values with $hover-color (or define another semantic variable if needed).
src/styles/index.scss
Outdated
| } | ||
|
|
||
| &:hover &__title { | ||
| color: #34568b; |
There was a problem hiding this comment.
The hover title color #34568b is hardcoded in the selector. The task requires special colors to be stored in variables ([CHECKLIST ITEM #2] "Repeated sizes and special colors are put to variables"). Consider adding a variable like $card-title-hover-color and use it here so colors are centralised.
mateacademy-ai-mentor
left a comment
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 requirement violations that will cause the tests to fail. First, in src/styles/index.scss, the button border is using the literal color #00acdc; per the checklist [CHECKLIST ITEM #2], repeated/special colors must come from variables, so this border should use $hover-color instead (e.g., border: 1px solid $hover-color;). Second, in src/index.html, the BUY link inside the first card currently has data-qa="card-button", but the task explicitly requires data-qa="card-hover" on that element, and automated tests depend on that exact attribute value. Related to that, you also need to remove or replace the old data-qa="card-button" attribute so that only the required data-qa attributes remain: data-qa="nav-hover" on the 4th nav link, data-qa="card" on the first card, and data-qa="card-hover" on the first card’s BUY button. Once you fix the color variable usage and correct the data-qa on the first card’s button, your solution should fully meet the task requirements. Overall, your use of SCSS variables, grid layout, responsive breakpoints, and hover effects looks solid—these last tweaks will align everything with the checklist and the automated tests.
✨ 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
src/styles/index.scss
Outdated
| font-size: 14px; | ||
| font-weight: 700; | ||
| border-radius: 4px; | ||
| border: 1px solid #00acdc; |
There was a problem hiding this comment.
This violates checklist item #2: "Repeated sizes and special colors are put to variables" — you used the literal color #00acdc for the button border here instead of the variable $hover-color. Replace the hardcoded #00acdc with border: 1px solid $hover-color; so the special hover color is consistently used from variables (src/styles/variables.scss) .
src/index.html
Outdated
| <button | ||
| class="card__button" | ||
| data-qa="card-button" |
There was a problem hiding this comment.
This violates the requirement: "add data-qa="card-hover" (not just hover) to the link Buy inside the first card" — the Buy button in the first card uses data-qa="card-button" instead. Change it to data-qa="card-hover" on the BUY button inside the first card so automated tests find the exact attribute value requested.
src/index.html
Outdated
|
|
||
| <button | ||
| class="card__button" | ||
| data-qa="card-button" |
There was a problem hiding this comment.
This violates the requirement: "remove old data-qa attributes" — data-qa="card-button" is an extra/incorrect attribute. Ensure you remove or replace any old data-qa attributes so only the explicitly required ones remain (data-qa="nav-hover" on the 4th nav link and data-qa="card" on the first card).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you learn, but there is one important issue to fix next. Your SCSS work is strong: you correctly used BEM, SCSS variables for colors and sizes, grid layout, responsive breakpoints (1/2/3/4 columns), and smooth 300ms hover transitions, which all align well with the task requirements and [CHECKLIST ITEM #1], [CHECKLIST ITEM #2], and [CHECKLIST ITEM #3]. In src/index.html, the main remaining problem is with the testing attribute on the first card: on line 105 the card wrapper currently has data-qa="card-hover", but per the checklist it must be data-qa="card" so automated tests can find the card correctly. To fix this, change the wrapper’s attribute to data-qa="card", keep the BUY button inside that same first card as data-qa="card-hover", and remove any other outdated data-qa values on that card so only these two remain. This small HTML adjustment will make your implementation fully aligned with the spec and prevent test failures, while preserving all the good layout and styling work you’ve already done. Great job overall on structure, semantics, and styling—this is very close to a perfect solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <main class="main"> | ||
| <div | ||
| class="card" | ||
| data-qa="card-hover" |
There was a problem hiding this comment.
This violates the Implementation Details / checklist requirement: "add data-qa="card" to the first card" — the first card currently has data-qa="card-hover" (line 105). Change the first card's attribute to data-qa="card" so tests can find it. Ensure the BUY button inside the first card keeps data-qa="card-hover" and that no other old data-qa attributes remain.
2 participants
No description provided.