-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
catalog #5022
base: master
Are you sure you want to change the base?
catalog #5022
Conversation
AndrewT98
commented
Jan 21, 2025
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your project! 🎉 While there are a few minor improvements that could be made, such as consolidating media queries and ensuring all colors and sizes are extracted into variables, these do not critically impact the functionality or core requirements of your code. Keep up the good work and consider these suggestions for future enhancements to make your code even more maintainable and efficient. 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/styles/blocks/card.scss
Outdated
border: 1px solid #f3f3f3; | ||
border-radius: 5px; | ||
|
||
translate: transform 300ms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property 'translate' seems to be incorrectly used here. It should be 'transition' to apply a transition effect on the 'transform' property.
src/styles/blocks/catalog.scss
Outdated
@media (min-width: 1024px) { | ||
.main { | ||
grid-template-columns: repeat(4, 200px); | ||
} | ||
} | ||
|
||
@media (min-width: 1024px) { | ||
.main { | ||
padding: 50px 40px 72px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two separate media queries for the 1024px breakpoint. Consider consolidating them into a single media query to improve code efficiency and maintainability.
src/styles/blocks/header.scss
Outdated
@media (width: 1200px) { | ||
.img-link { | ||
margin-right: 419px; | ||
} | ||
} | ||
|
||
@media (width: 1024px) { | ||
.img-link { | ||
margin-right: 243px; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using exact widths in media queries (e.g., @media (width: 1200px)
) is not ideal for responsive design. Consider using min-width
or max-width
to create more flexible breakpoints that adapt to a range of screen sizes.
&__star:nth-child(-n + 4) { | ||
background-image: url(../images/star-active.svg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nth-child(-n + 4)
selector is used to apply a different background image to the first four stars. Ensure that this matches the intended design, as it might not be immediately clear why only the first four stars are styled differently. Consider adding a comment to clarify the purpose.