Skip to content
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

make catalog #4994

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

make catalog #4994

wants to merge 2 commits into from

Conversation

kepchuk1234
Copy link

  • 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 smooth

  • Code follows all the Code Style Rules ❗️

Copy link

@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.

It looks like there are a couple of critical issues that need to be addressed before we can approve the pull request. The data-qa attribute inconsistency and the typo in the class name nav__-item could impact the functionality and consistency of your project. Please correct these issues to ensure everything works smoothly. Once you've made these changes, feel free to resubmit. 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.

src/index.html Outdated
</a>
</li>

<li class="nav__-item">

Choose a reason for hiding this comment

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

There is a typo in the class name nav__-item. It should be nav__item to maintain consistency with the other list items.

src/index.html Outdated
<a
href="#"
class="card__link"
data-qa="hover"

Choose a reason for hiding this comment

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

The data-qa attribute here is hover, which is inconsistent with the other card links that use card-hover. Consider using a consistent naming convention for data-qa attributes.

src/index.html Outdated
<a
href="#"
class="card__link"
data-qa="hover"

Choose a reason for hiding this comment

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

The data-qa attribute here is hover, which is inconsistent with the other card links that use card-hover. Consider using a consistent naming convention for data-qa attributes.

src/index.html Outdated
<a
href="#"
class="card__link"
data-qa="hover"

Choose a reason for hiding this comment

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

The data-qa attribute here is hover, which is inconsistent with the other card links that use card-hover. Consider using a consistent naming convention for data-qa attributes.

src/index.html Outdated
<a
href="#"
class="card__link"
data-qa="hover"

Choose a reason for hiding this comment

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

The data-qa attribute here is hover, which is inconsistent with the other card links that use card-hover. Consider using a consistent naming convention for data-qa attributes.

src/index.html Outdated
<a
href="#"
class="card__link"
data-qa="hover"

Choose a reason for hiding this comment

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

The data-qa attribute here is hover, which is inconsistent with the other card links that use card-hover. Consider using a consistent naming convention for data-qa attributes.

src/index.html Outdated
<a
href="#"
class="card__link"
data-qa="hover"

Choose a reason for hiding this comment

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

The data-qa attribute here is hover, which is inconsistent with the other card links that use card-hover. Consider using a consistent naming convention for data-qa attributes.

src/index.html Outdated
<a
href="#"
class="card__link"
data-qa="hover"

Choose a reason for hiding this comment

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

The data-qa attribute here is hover, which is inconsistent with the other card links that use card-hover. Consider using a consistent naming convention for data-qa attributes.

text-transform: uppercase;
text-decoration: none;
text-align: center;
transition: all, $transition-duration;

Choose a reason for hiding this comment

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

The transition property is missing a specific property to transition. It currently uses all, which can lead to performance issues. Consider specifying the properties you want to transition, such as background-color and color, along with the $transition-duration.

@@ -0,0 +1,7 @@
@mixin on-hover($property, $value) {
transition: $transition-duration;

Choose a reason for hiding this comment

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

The transition property should specify the CSS property being transitioned, rather than using a generic transition. Consider specifying the property, such as color or background-color, along with $transition-duration.

Copy link

@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.


Great job on addressing the previous issues and ensuring that the HTML and SCSS files are well-structured and follow the BEM methodology. The use of variables and mixins enhances maintainability and consistency across the project. However, there are a few areas to improve:

  • Transition Property: Ensure that the transition property specifies only the CSS properties that need to be animated, such as background-color and color, to improve performance. Avoid using all as it can lead to unnecessary performance overhead.

  • Data Attributes Consistency: Make sure that the data-qa attributes are consistently applied across similar elements, such as the card-hover, to maintain uniformity and facilitate testing.

These improvements will enhance the functionality and performance of your 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
href="#"
class="card__link"
data-qa="card-hover"

Choose a reason for hiding this comment

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

The data-qa attribute card-hover is used here, but it is missing in other similar elements. Ensure consistent usage of data-qa attributes across similar elements to maintain uniformity and facilitate testing.

text-transform: uppercase;
text-decoration: none;
text-align: center;
transition: all, $transition-duration;

Choose a reason for hiding this comment

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

The transition property should specify which CSS properties to animate. Using all can lead to performance issues as it will apply to all properties, even those that do not change. Consider specifying only the properties that need to be animated, such as background-color and color.

@@ -0,0 +1,7 @@
@mixin on-hover($property, $value) {
transition: $property $transition-duration;

Choose a reason for hiding this comment

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

The transition property should specify which CSS properties to animate. Consider using transition: #{$property} $transition-duration; to ensure that only the intended property is animated, which can improve performance.

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