add task solution v1#6119
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
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:
- 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). - Hardcoded Colors: Several colors are hardcoded, which violates a checklist requirement [CHECKLIST ITEM #2]. Please define variables in
variables.scssfor the hover color#34568bincard.scssand the link color#060b35inheader.scss. - Missing Transition: The color change on the card title is instant. All hover effects should be smooth, so please add a
transitionproperty to the card title to meet the 300ms requirement [CHECKLIST ITEM #5]. - Discouraged CSS Unit: The header uses
width: 100vw, which can cause unexpected scrollbars. Please change this to100%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
| @@ -0,0 +1,72 @@ | |||
| .header { | |||
| width: 100vw; | |||
There was a problem hiding this comment.
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 ❗️]'.
| display: flex; | ||
| margin: 0 10px; | ||
| text-decoration: none; | ||
| color: #060b35; |
There was a problem hiding this comment.
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.
| &:hover { | ||
| color: #34568b; | ||
| } |
There was a problem hiding this comment.
This hover effect is not implemented as required.
- 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). - Additionally, this violates checklist item Vitalii Pshychenko #2. Special colors like
#34568bshould be defined as a variable invariables.scssfor better maintainability.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
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
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
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
|
|
||
| .catalog { | ||
| display: grid; | ||
| grid-template-columns: repeat(1, 200px); |
There was a problem hiding this comment.
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.
| $white-color: #fff; | ||
| $star-size: 16px; | ||
| $star-spacing: 4px; | ||
| $hover-card-name-color: #34568b; |
There was a problem hiding this comment.
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.
❗️ Copy this
Checklistto thePull Requestdescription after links, and put- [x]before each point after you checked it.:hoverare smooth