solution with failed tests#6281
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 there are a few critical mismatches with the task description and checklist that will cause tests and visual checks to fail. First, data-qa="nav-hover" is currently applied to the first three nav links as well as the fourth; the requirement is that only the 4th nav link (Laptops & computers) has this attribute for testing, so you need to remove it from the other links [CHECKLIST ITEM #1]. Second, the cards container <main> is using the class card, while the actual card block is card-monitor; the description expects a Card block with a selector like .card:hover .card__title, so you should introduce a separate catalog block for <main> (e.g., .catalog) and align the card block/element names with the described .card/.card__title structure [CHECKLIST ITEM #1]. Third, repeated colors and sizes (e.g., #060b35, #616070, #34568b, #00acdc, #fff, #f3f3f3, card width, button dimensions, gaps, paddings) are hard-coded multiple times; these must be moved into SCSS variables and reused across the styles [CHECKLIST ITEM #2]. Finally, the spec explicitly says to add data-qa="card-hover" to the link "Buy" inside the first card; you currently use a <button>, so it’s safer to switch this control to an <a> or otherwise match the expected element so tests can find it reliably. Once you fix the data-qa placement, BEM/block naming, and SCSS variable usage, the rest of your solution looks solid and should pass the checks—your layout, grid breakpoints, and hover transitions are all implemented well.
✨ 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
| <a | ||
| href="#" | ||
| class="nav__anchor is-active" | ||
| data-qa="nav-hover" |
There was a problem hiding this comment.
According to the description: add data-qa="nav-hover" (not just "hover") to the 4th nav link for testing (Laptops & computers). Here you’ve added data-qa="nav-hover" to the first link (Apple), which violates that requirement.
| <a | ||
| class="nav__anchor" | ||
| href="#" | ||
| data-qa="nav-hover" |
There was a problem hiding this comment.
This data-qa="nav-hover" is on the second nav link (Samsung). The description requires the attribute only on the 4th nav link (Laptops & computers), so having it here violates that requirement.
| <a | ||
| class="nav__anchor" | ||
| href="#" | ||
| data-qa="nav-hover" |
There was a problem hiding this comment.
Again, data-qa="nav-hover" is applied to the third nav link (Smartphones). The task explicitly says only the 4th nav link should have this data-qa, so this is a mismatch.
| <li class="nav__item"> | ||
| <a | ||
| class="nav__anchor" | ||
| data-qa="nav-hover" |
There was a problem hiding this comment.
Here the data-qa="nav-hover" is correctly on the 4th nav link. However, because you also added it to the 1st–3rd links, the implementation violates the description requirement that only the 4th nav link carries this attribute for testing.
| </ul> | ||
| </nav> | ||
| </header> | ||
| <main class="card"> |
There was a problem hiding this comment.
<main class="card"> uses the class name card for the catalog container. The description distinguishes between a Card block and a catalog container (cards grid), and also mentions .card:hover .card__title. Using card on <main> while the actual card block is named card-monitor leads to a BEM and semantics mismatch with the description’s Card block. This violates checklist item #1: All components follow BEM and use SCSS in how blocks are named and structured.
| </h3> | ||
| <small class="card-monitor__code">Product code: 195434</small> | ||
| </div> | ||
| <div class="card-monitor__rating"> | ||
| <div class="stars stars--4 card-monitor__stars"> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> |
There was a problem hiding this comment.
Here background-color: #00acdc; and color: #fff; repeat the same special colors you already use in other places (and you have --blue defined). Checklist item #2 requires repeated sizes and special colors to be put into variables, so these should be replaced with SCSS variables instead of hard-coded values.
| <button | ||
| type="button" | ||
| class="card-monitor__button" | ||
| > |
There was a problem hiding this comment.
The hover styles hard-code #00acdc and #fff again. Combined with other occurrences, this goes against checklist item #2: Repeated sizes and special colors are put to variables. Define SCSS variables for these colors and reuse them here.
| padding: 0 50px 0 0; | ||
|
|
||
| &__logo-link { | ||
| display: flex; |
There was a problem hiding this comment.
According to the description and checklist item #1: "remove old data-qa attributes" and "add data-qa="nav-hover" to the 4th nav link (Laptops & computers)". Here data-qa="nav-hover" is applied to the first link, which violates the requirement that only the 4th nav link should have this attribute.
|
|
||
| .nav__list { | ||
| display: flex; | ||
| align-items: center; |
There was a problem hiding this comment.
This data-qa="nav-hover" on the Samsung link also violates the requirement: only the 4th nav link (Laptops & Computers) should have data-qa="nav-hover". Other nav links must not have this attribute.
|
|
||
| &:first-child { | ||
| margin-left: 0; | ||
| } |
There was a problem hiding this comment.
Similarly, data-qa="nav-hover" on the Smartphones link breaks the requirement that only the 4th nav link should have this data-qa for testing. Consider removing this attribute here.
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 is still a critical mismatch with the required navigation hover color, which is part of the task’s functional spec and will affect tests. Specifically, in src/styles/index.scss your .nav__anchor:hover currently uses color: var(--blue) (which is #34568b), but the requirement states that nav link text on :hover must be #00acdc (the light blue). To fix this, change that rule to use color: var(--light-blue) (assuming --light-blue is defined as #00acdc in :root), so the hover behavior exactly matches the description and checklist. Aside from this, your structure, BEM usage, grid layout, data-qa placement, and hover transitions look well implemented and aligned with the task. As a minor improvement (not blocking), consider correcting small typos in variable names like --darc-blue and --hight-grey for clarity. Once the nav hover color is updated to the specified #00acdc, your solution should fully meet the 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 { | ||
| color: var(--blue); |
There was a problem hiding this comment.
This violates the hover requirement from the description: navigation link text color on :hover must be #00acdc, but here you set it to var(--blue) (#34568b). Consider changing this to var(--light-blue) to match the task spec.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved — it matches the catalog task requirements very well and demonstrates solid use of semantic HTML, BEM, SCSS, and responsive grid layout. The header, navigation, stars component, and card block are structured cleanly, and your hover interactions (nav links, cards, and buttons) follow the required colors, timing, and smooth transitions [CHECKLIST ITEM #5]. The data-qa attributes are correctly applied to the 4th nav link, the first card, and its Buy button, and your media queries produce the correct 1/2/3/4 column layouts with the specified gaps and paddings [CHECKLIST ITEM #3]. Visual regression tests showed some differences from the reference screenshots, but this is an educational override to help you continue progressing while you learn; if you want to refine further, compare your rendered page to the reference and adjust spacing, font sizes, or states until they match more closely. A minor improvement you could make later is to clean up small typos in custom property names (for example, renaming something like --hight-grey to --light-grey) to keep your codebase clearer [CHECKLIST ITEM #2]. Overall, you’ve done a thoughtful job implementing the catalog page, and these last visual polish steps are great opportunities to practice attention to detail in UI work.
✨ 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
3 participants
DEMO LINK