Skip to content

add task solution v1#6119

Open
DmitriyS2k wants to merge 3 commits intomate-academy:masterfrom
DmitriyS2k:develop
Open

add task solution v1#6119
DmitriyS2k wants to merge 3 commits intomate-academy:masterfrom
DmitriyS2k:develop

Conversation

@DmitriyS2k
Copy link
Copy Markdown

❗️ Copy this Checklist to the Pull Request description after links, and put - [x] before each point after you checked it.

  • 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 ❗️

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

Great job on the overall structure, BEM implementation, and the responsive grid layout! Your HTML is excellent. However, a few adjustments are needed in your SCSS files to fully meet all the task requirements before this can be approved.

Here are the specific points to address:

  1. Incorrect Card Title Hover: In src/styles/card.scss, the title's color changes only when hovering over the title itself. The requirement is for the color to change when the entire card is hovered. You'll need to adjust the CSS selector to target the title when the parent card is hovered (e.g., .card:hover .card__name).
  2. Hardcoded Colors: Several colors are hardcoded, which violates a checklist requirement [CHECKLIST ITEM #2]. Please define variables in variables.scss for the hover color #34568b in card.scss and the link color #060b35 in header.scss.
  3. Missing Transition: The color change on the card title is instant. All hover effects should be smooth, so please add a transition property to the card title to meet the 300ms requirement [CHECKLIST ITEM #5].
  4. Discouraged CSS Unit: The header uses width: 100vw, which can cause unexpected scrollbars. Please change this to 100% as recommended by the project's style guide [CHECKLIST ITEM #6].

Please make these changes, and I'll be happy to review your work again!


✨ 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/header.scss Outdated
@@ -0,0 +1,72 @@
.header {
width: 100vw;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using 100vw for width can cause unexpected horizontal scrollbars. The project's code style guide recommends using 100% instead for page containers. This violates checklist item #6: 'Code follows all the [Code Style Rules ❗️]'.

Comment thread src/styles/header.scss Outdated
display: flex;
margin: 0 10px;
text-decoration: none;
color: #060b35;
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 color is hardcoded. According to the task requirements (Checklist item #2: 'Repeated sizes and special colors are put to variables'), this color should be stored in a variable. A suitable variable $main-card-text-color already exists in variables.scss.

Comment thread src/styles/card.scss Outdated
Comment on lines +41 to +43
&:hover {
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 hover effect is not implemented as required.

  1. The requirement is to change the title's color when the entire card is hovered (.card:hover .card__name), but the current implementation changes it only when the title itself is hovered (.card__name:hover).
  2. Additionally, this violates checklist item Vitalii Pshychenko #2. Special colors like #34568b should be defined as a variable in variables.scss for better maintainability.

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

Excellent work on this task! You've successfully addressed all the previous feedback, and your code is clean and well-structured.

However, changes are needed because the automated build is failing due to a visual layout issue. The problem is in src/styles/main.scss where your .catalog grid uses 1fr for column widths. Since the cards have a fixed width of 200px, this creates too much space between them on larger screens, causing the visual test to fail.

To fix this, please update your grid-template-columns to use the card's fixed width. For example:

// src/styles/main.scss
.catalog {
  // ...
  grid-template-columns: repeat(1, 200px);
}

@media (min-width: 1024px) {
  .catalog {
    grid-template-columns: repeat(4, 200px);
  }
}

Applying this change for all media queries will resolve the test failure [CHECKLIST ITEM #6]. You're very close to finishing!


✨ 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

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

Excellent work resolving the layout issue from the previous review! The catalog grid now functions perfectly across all screen sizes.

I am approving this submission as an educational override to help you progress, but please pay close attention to this feedback for future tasks. The main remaining issue is that the card width (200px) is hardcoded in multiple places (main.scss and card.scss), which violates the requirement to use variables for repeated values ([CHECKLIST ITEM #2]).

To improve this, you should declare a variable like $card-width: 200px; in variables.scss and then use that variable in your other style files. This practice makes your code much cleaner and easier to maintain.

You've made great progress on this task. Keep up the fantastic work!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/styles/main.scss

.catalog {
display: grid;
grid-template-columns: repeat(1, 200px);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The value 200px is repeated on lines 14, 23, 29, and 35, and is also used in card.scss. This violates checklist item #2: 'Repeated sizes and special colors are put to variables'. Please create a variable for the card width in variables.scss and use it in all relevant places.

Comment thread src/styles/variables.scss
$white-color: #fff;
$star-size: 16px;
$star-spacing: 4px;
$hover-card-name-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 is a great start! To fully comply with the requirement to use variables for repeated values, please add a variable for the card width (200px) here. This value is used in both card.scss and main.scss.

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.

2 participants