-
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
catalog #5002
base: master
Are you sure you want to change the base?
catalog #5002
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.
There are a few critical issues that need to be addressed before we can approve the pull request. The transition duration and card hover scale do not meet the specified requirements, and there is a missing data-qa
attribute that affects the functionality. Additionally, ensure that all CSS properties have appropriate units to prevent rendering issues. Once these are resolved, your code will be in great shape! Keep up the good work and don't hesitate to reach out if you need any help. 😊
✨ 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
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> | ||
|
||
<div class="card"> | ||
<div class="card__image-container"> | ||
<img | ||
src="images/imac.jpeg" | ||
class="card__image" | ||
alt="mac-photo" | ||
/> | ||
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> | ||
|
||
<div class="card"> | ||
<div class="card__image-container"> | ||
<img | ||
src="images/imac.jpeg" | ||
class="card__image" | ||
alt="mac-photo" | ||
/> | ||
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> |
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 product details, including the image, title, product code, reviews, and price, are repeated across multiple cards. If this is not intentional, consider using a templating engine or JavaScript to dynamically generate these cards with varying content to avoid redundancy and potential maintenance issues.
src/styles/components/card/card.scss
Outdated
color: $font-color-secondary; | ||
} | ||
&-number { | ||
font-size: 16; |
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 font-size property should include a unit (e.g., 'px' or 'em'). Currently, it is set to '16' without a unit, which might cause unexpected behavior in some browsers.
src/styles/components/card/card.scss
Outdated
align-items: center; | ||
width: 100%; | ||
border: 1px solid $button-bcg-color; | ||
border-radius: 5; |
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 border-radius property should include a unit (e.g., 'px'). Currently, it is set to '5' without a unit, which might cause unexpected behavior in some browsers.
src/styles/components/card/card.scss
Outdated
&--styles { | ||
text-transform: uppercase; | ||
text-decoration: none; | ||
font-size: 14; |
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 font-size property should include a unit (e.g., 'px' or 'em'). Currently, it is set to '14' without a unit, which might cause unexpected behavior in some browsers.
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.
Review Summary
Great effort on addressing most of the issues from the previous review! 🎉 Your code is generally well-structured, and you've made significant progress. However, there are a few critical points that need attention to ensure your project fully meets the requirements:
-
Transition Duration and Card Hover Scale: The transition duration for hover effects should be updated to 300ms, and the hover scale should be set to 1.05 to align with the task specifications. This will enhance the user experience and ensure consistency.
-
CSS Properties Units: Double-check that all CSS properties have appropriate units. This is crucial for preventing rendering issues and maintaining a consistent design across different devices.
-
Data-qa Attributes: Ensure that each card element in your HTML has a
data-qa
attribute for testing purposes. This is essential for maintaining a robust testing framework.
Despite these issues, the decision to approve your pull request is made to help you move forward with your learning journey. Please review the comments and address these points in your future work to avoid similar problems. Keep up the good work, and continue to improve your code! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<div class="card"> | ||
<div class="card__image-container"> | ||
<img | ||
src="images/imac.jpeg" | ||
class="card__image" | ||
alt="mac-photo" | ||
/> | ||
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars card__stars--4"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> |
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 data-qa
attribute is missing from this card element. Ensure that each card has a data-qa
attribute for testing purposes, similar to the first card.
<div class="card"> | ||
<div class="card__image-container"> | ||
<img | ||
src="images/imac.jpeg" | ||
class="card__image" | ||
alt="mac-photo" | ||
/> | ||
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars card__stars--4"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> |
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 data-qa
attribute is missing from this card element. Ensure that each card has a data-qa
attribute for testing purposes, similar to the first card.
<div class="card"> | ||
<div class="card__image-container"> | ||
<img | ||
src="images/imac.jpeg" | ||
class="card__image" | ||
alt="mac-photo" | ||
/> | ||
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars card__stars--4"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> |
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 data-qa
attribute is missing from this card element. Ensure that each card has a data-qa
attribute for testing purposes, similar to the first card.
<div class="card"> | ||
<div class="card__image-container"> | ||
<img | ||
src="images/imac.jpeg" | ||
class="card__image" | ||
alt="mac-photo" | ||
/> | ||
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars card__stars--4"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> |
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 data-qa
attribute is missing from this card element. Ensure that each card has a data-qa
attribute for testing purposes, similar to the first card.
<div class="card"> | ||
<div class="card__image-container"> | ||
<img | ||
src="images/imac.jpeg" | ||
class="card__image" | ||
alt="mac-photo" | ||
/> | ||
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars card__stars--4"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> |
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 data-qa
attribute is missing from this card element. Ensure that each card has a data-qa
attribute for testing purposes, similar to the first card.
<div class="card"> | ||
<div class="card__image-container"> | ||
<img | ||
src="images/imac.jpeg" | ||
class="card__image" | ||
alt="mac-photo" | ||
/> | ||
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars card__stars--4"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> |
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 data-qa
attribute is missing from this card element. Ensure that each card has a data-qa
attribute for testing purposes, similar to the first card.
<div class="card"> | ||
<div class="card__image-container"> | ||
<img | ||
src="images/imac.jpeg" | ||
class="card__image" | ||
alt="mac-photo" | ||
/> | ||
</div> | ||
<h1 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h1> | ||
<h3 class="card__code">Product code: 195434</h3> | ||
<div class="card__stars card__stars--4"> | ||
<div class="card__stars-container"> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
<div class="card__stars-star"></div> | ||
</div> | ||
<h3 class="card__stars-reviews">Reviews: 5</h3> | ||
</div> | ||
<div class="card__price"> | ||
<h2 class="card__price-text">Price:</h2> | ||
<h2 class="card__price-number">$2,199</h2> | ||
</div> | ||
<div class="card__button-container"> | ||
<a | ||
href="#" | ||
class="card__button card__button--styles" | ||
> | ||
Buy | ||
</a> | ||
</div> | ||
</div> |
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 data-qa
attribute is missing from this card element. Ensure that each card has a data-qa
attribute for testing purposes, similar to the first card.
border-radius: 5px; | ||
width: 200px; | ||
padding: 32px 16px 16px; | ||
transition-duration: 200ms; |
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 transition duration should be set to 300ms as per the task requirements. Please update this value to ensure consistency with the project specifications.
transition-duration: 200ms; | ||
|
||
&:hover { | ||
transform: scale(120%); |
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 hover scale should be set to 1.05 as per the task requirements. Please update this value to ensure consistency with the project specifications.
.container { | ||
display: grid; | ||
justify-content: center; | ||
gap: 46px 48px; |
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.
Ensure that the gap
property is set according to the design specifications. Verify that the values align with the intended layout and adjust if necessary.
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 ❗️