-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Develop #4999
base: master
Are you sure you want to change the base?
Develop #4999
Conversation
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.
Thank you for your hard work on this project! 😊 While your code is mostly on the right track, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Class Naming Conventions: The repeated use of
card__price-price_2199
and similar class names is not following a clear and consistent naming convention. It's important to use descriptive and meaningful class names to improve code readability and maintainability. -
Hover State Management: The use of the
.hover
class for managing hover states should be replaced with CSS pseudo-classes like:hover
. This will help keep your HTML clean and your CSS more efficient. -
Box-Sizing Property: There are conflicting
box-sizing
declarations. Please ensure that the intended value is applied and remove any redundant declarations.
Please address these issues to ensure your code is clean, maintainable, and follows best practices. Once these changes are made, we can proceed with approving your pull request. Keep up the great work! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
</a> | ||
<a | ||
href="#" | ||
class="navigation__link hover" |
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 class hover
seems to be a state-related class, which is typically managed by CSS pseudo-classes like :hover
. Consider removing it from the HTML and handling hover states in your CSS.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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 class card__price-price_2199
is not following a clear naming convention. Consider using a more descriptive name that reflects its purpose or style, rather than the price value.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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.
Similar to line 114, the class card__price-price_2199
should follow a consistent naming convention. Avoid using specific values in class names unless necessary for styling purposes.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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.
Again, the class card__price-price_2199
appears. Ensure that class names are consistent and meaningful across your HTML.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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 class card__price-price_2199
is repeated. Consider refactoring your CSS to avoid redundancy and improve maintainability.
} | ||
|
||
body, | ||
a { |
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.
Combining body
and a
selectors is fine for shared styles, but ensure that this doesn't lead to unintended styling for links. If links need different styles, consider separating these rules.
src/styles/blocks/catalog.scss
Outdated
box-sizing: border-box; | ||
box-sizing: inherit; |
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 box-sizing
property is set twice with different values. The second declaration (box-sizing: inherit;
) will override the first one (box-sizing: border-box;
). Ensure this is intentional or remove the redundant declaration.
@include font(12px, 400); | ||
} | ||
|
||
&__price-price_2199 { |
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 class &__price-price_2199
is not following a clear naming convention. Consider using a more descriptive name that reflects its purpose or style, rather than the price value.
&__link.is-active:hover, | ||
&__link.hover:hover { |
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 class &__link.hover
is used for hover states, which should typically be handled using CSS pseudo-classes like :hover
. Consider removing the .hover
class from your HTML and managing hover states purely with CSS.
background-repeat: no-repeat; | ||
} | ||
|
||
&__starN { |
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 class &__starN
is not very descriptive. Consider renaming it to something more meaningful, such as &__star--inactive
or &__star-empty
, to clearly indicate its purpose.
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 submission! 🎉 While your code is mostly well-structured, there are a few areas for improvement:
-
Class Naming Conventions: The class name
card__price-price_2199
is not following a clear and consistent naming convention. Consider using more descriptive and meaningful class names to improve code readability and maintainability. -
Hover State Management: The use of the
.hover
class for managing hover states is not recommended. Instead, use CSS pseudo-classes like:hover
to keep your HTML cleaner and your CSS more efficient. -
Universal Selector: Using the universal selector
*
to reset styles can lead to performance issues and unintended side effects. Consider a more targeted approach or a CSS reset library.
These changes will help you write cleaner and more maintainable code. Keep up the good work, and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
</a> | ||
<a | ||
href="#" | ||
class="navigation__link hover" |
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 use of the hover
class for managing hover states is not recommended. Instead, use CSS pseudo-classes like :hover
to manage hover states. This will keep your HTML cleaner and your CSS more efficient.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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 class name card__price-price_2199
is not following a clear and consistent naming convention. Consider using a more descriptive and meaningful class name to improve code readability and maintainability.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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 class name card__price-price_2199
is repeated here. Ensure that class names are consistent and meaningful across your HTML.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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 class name card__price-price_2199
is repeated here. Consider using a more descriptive and meaningful class name.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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 class name card__price-price_2199
is repeated here. Ensure that class names are consistent and meaningful.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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 class name card__price-price_2199
is repeated here. Consider using a more descriptive and meaningful class name.
</div> | ||
<div class="card__price"> | ||
<p class="card__price-price">Price:</p> | ||
<p class="card__price-price_2199">$2,199</p> |
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 class name card__price-price_2199
is repeated here. Ensure that class names are consistent and meaningful.
@@ -0,0 +1,12 @@ | |||
* { |
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 the universal selector *
to reset styles can lead to performance issues and unintended side effects. Consider using a more targeted approach, such as resetting only the elements you need or using a CSS reset library.
@include font(12px, 400); | ||
} | ||
|
||
&__price-price_2199 { |
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 class name &__price-price_2199
is not following a clear and consistent naming convention. Consider using a more descriptive and meaningful class name to improve code readability and maintainability.
} | ||
|
||
&__link.is-active:hover, | ||
&__link.hover:hover { |
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 use of the .hover
class for managing hover states is not recommended. Instead, use CSS pseudo-classes like :hover
to manage hover states. This will keep your HTML cleaner and your CSS more efficient.
DEMO LINK
TEST REPORT LINK
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 smoothCode follows all the Code Style Rules ❗️