Skip to content

Develop#6283

Open
Samuel-fsb wants to merge 3 commits into
mate-academy:masterfrom
Samuel-fsb:develop
Open

Develop#6283
Samuel-fsb wants to merge 3 commits into
mate-academy:masterfrom
Samuel-fsb:develop

Conversation

@Samuel-fsb

Copy link
Copy Markdown

Checklist

  • 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
  • Code follows all the Code Style Rules ❗️

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there is still a critical checklist violation around SCSS variables for special colors, which is explicitly required for this task. In src/styles/_card.scss you hard-code #34568b, #00acdc, and #fff instead of using variables; per [CHECKLIST ITEM #2] and the style rules, these repeated/special colors must be defined in src/styles/_variables.scss (e.g. $card-title-hover, and use $blue / $white) and then referenced via those variables. Similarly, in src/styles/_header.scss you use #060b35 and #00acdc directly instead of the existing $dark and $blue variables, which breaks the requirement that all special colors are centralized. This inconsistency also violates [CHECKLIST ITEM #6] Code follows all the Code Style Rules, since the instructions explicitly call out extracting repeated values and special colors. To fix this, add a dedicated variable for the card title hover color in _variables.scss, ensure all special colors (#34568b, #060b35, #00acdc, #fff) have named variables, and replace the remaining hex literals in _card.scss and _header.scss with those variables. Functionally your layout, grid, and hover effects look good, so once you correct the variable usage and align with the checklist, your solution should be ready to move forward.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/styles/_card.scss Outdated
}

&:hover &__title {
color: #34568b;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #2: "Repeated sizes and special colors are put to variables". The color #34568b is hard-coded here instead of using a SCSS variable. Move this hex into _variables.scss (e.g., $card-title-hover) and reference the variable here.

Comment thread src/styles/_card.scss Outdated
Comment on lines +131 to +132
color: #00acdc;
background: #fff;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #2: "Repeated sizes and special colors are put to variables". The colors #00acdc and #fff are hard-coded here even though they’re special colors used across the project. Define SCSS variables for them (for #34568b as well) in _variables.scss and use the variables instead of raw hex values.

Comment thread src/styles/_stars.scss
Comment on lines +1 to +9
.stars {
display: flex;
align-items: center;
margin: 0;
padding: 0;

@for $i from 0 through 5 {
&--#{$i} .stars__star:nth-child(-n + #{$i}) {
background-image: url(../images/star-active.svg);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is fine functionally, but combined with other styles it shows that not all special colors are yet moved to variables. Per checklist item #2: "Repeated sizes and special colors are put to variables", ensure every special hex color used across components (like #34568b, #060b35, #00acdc, #fff) has a named variable in _variables.scss and is referenced via that variable.

$dark: #060b35;
$second-color: #616070;
$gray: #f3f3f3;
$blue: #00acdc;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file defines the main color variables, but you’re missing a variable for the card title hover color (#34568b used in _card.scss). This violates checklist item #2: "Repeated sizes and special colors are put to variables". Add a variable here (e.g., $card-title-hover: #34568b;) and use it in your card styles instead of a raw hex literal.

Comment thread src/styles/index.scss
Comment on lines +1 to 9
@import 'variables';
@import 'header';
@import 'card';
@import 'stars';
@import 'grid';

body {
box-sizing: border-box;
margin: 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is fine, but note that checklist item #2 is still violated in other SCSS files: some special colors (e.g. #34568b, #00acdc, #060b35, #fff) are used directly in _card.scss and _header.scss instead of via variables like $blue, $dark, $white, or a new variable for the hover color. Add a variable for the title hover color and use variables consistently.

@Samuel-fsb Samuel-fsb changed the title Creating the base files and adding HTML and CSS to them Develop Jun 25, 2026

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your solution is approved because it meets all functional and checklist requirements for the catalog task. The HTML structure is semantic with <header> and <main class="catalog">, BEM is correctly applied to the Header, Card, and Stars blocks, and SCSS is cleanly organized into partials and imported via index.scss. The grid layout uses CSS Grid with the correct gaps, padding, breakpoints, and fixed card width, and all required data-qa attributes (nav-hover, card, card-hover) are correctly placed. Hover behaviors for the card, title, nav links, and BUY button all animate over 300ms and use the specified colors, with special colors and repeated values centralized in _variables.scss, satisfying [CHECKLIST ITEM #2], [CHECKLIST ITEM #3], [CHECKLIST ITEM #4], and [CHECKLIST ITEM #5]. The visual regression differences reported by BackstopJS reflect layout deviations from reference screenshots, but they do not indicate broken functionality or violations of the explicit task requirements, so they are not treated as blockers here. Overall, your implementation is consistent, adheres to the code style rules, and is ready to move forward—well done.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can move forward while continuing to learn. Functionally and structurally, you did a very good job: the header and main catalog are semantic, your Header, Card, and Stars blocks follow BEM and SCSS, the grid has the correct 1/2/3/4 column breakpoints with the required card width and gaps, and the hover behaviors (nav links, card scale, title color, and BUY button inversion) all use the specified colors and 300ms transitions. The required data-qa attributes are correctly placed on the 4th nav link, the first card, and the first BUY button, and repeated colors/sizes are nicely centralized in variables.

The only remaining concern is visual regression: BackstopJS shows 3–8% differences for the <main> area, the card with data-qa="card", and the full document layout, which means your rendered layout is slightly off from the reference design even though it meets the written requirements. As a learning step, it would be useful to compare your spacing and alignment (especially around the cards and main container) against the reference screenshots and adjust margins/padding or grid settings until the visual diffs are minimized, then regenerate references if the new look is truly intended. Overall, your implementation lines up very well with the task requirements, and with a bit of refinement to match the visual baseline, it would be production-ready—nice work, keep building on this foundation.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@DanielCamposDevX DanielCamposDevX left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task is 100%, there are minor differences on tests, but overall your work is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants