-
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
add solution #4831
base: master
Are you sure you want to change the base?
add solution #4831
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.
Hello! It looks like there are a few critical issues that need addressing before we can move forward. Specifically, the implementation of the 'buy' button and the stars block need to be revised to ensure they are semantically correct and meet the task requirements. Additionally, make sure to link to the compiled CSS file instead of the SCSS file. Keep up the good work, and with these changes, your code will be in great shape! 🛠💼
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -12,7 +12,48 @@ | |||
href="./styles/index.scss" |
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.
Incorrect href attribute value for a stylesheet link. SCSS is a preprocessor and you should link to the compiled CSS file, not the SCSS file. Make sure you compile your SCSS to CSS and link to the resulting CSS file.
<div class="product-card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/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.
Avoid using
to create space or layout designs. If you need to create a visual separation or spacing, use CSS margin or padding.
<div class="stars"> | ||
<div class="stars-4"> | ||
<div class="stars-4__star"></div> | ||
<div class="stars-4__star"></div> | ||
<div class="stars-4__star"></div> | ||
<div class="stars-4__star"></div> | ||
<div class="stars-4__star"></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 stars block should be implemented with CSS background images, not with multiple div elements. Also, you should use a modifier class on the parent block instead of creating a separate block for 'stars-4'.
href="#" | ||
data-qa="hover" | ||
> | ||
<div class="button-buy">buy</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 'buy' button should be implemented as a element inside the link (), not as a
height: 134px; | ||
margin-bottom: 40px; | ||
align-self: center; | ||
margin-inline: 3px; |
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 'margin-inline: 3px;' property seems unnecessary here, as the image is already centered using 'align-self: center;'. It might be causing the image to shift slightly, which could affect the design consistency.
|
||
.stars { | ||
display: flex; | ||
justify-content: space-between; |
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.
According to the Code Style Rules, there should not be a space between property and value. In 'justify-content: space-between;' the space after ':' should be removed.
|
||
.stars-4 { | ||
display: flex; | ||
justify-content: space-between; |
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 '.stars-4' class duplicates the properties of the '.stars' class. Consider combining them to reduce redundancy and improve maintainability.
.stars-4__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.
Using ':nth-child(-n + 4)' is a clever way to apply styles to the first four elements, but ensure that this approach aligns with the dynamic nature of the stars component. If the number of active stars should be variable, consider a more flexible solution that adapts to different ratings.
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.
Please address the issues mentioned regarding BEM conventions, the use of non-standard properties, global styles, semantic HTML, and correct linking to stylesheets. These are important for the maintainability, functionality, and compatibility of your code. Keep up the good work, and with a few tweaks, your code will be even better! 🛠️💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -12,7 +12,48 @@ | |||
href="./styles/index.scss" |
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.
Stylesheets should be linked with a .css extension, not .scss. The browser cannot understand SCSS files directly; they need to be compiled to CSS first.
<div class="product-card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/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.
Avoid using
to create a new line for text content. Consider wrapping the text into separate elements or using CSS to manage the layout.
|
||
<div class="product-card__code">Product code: 195434</div> | ||
|
||
<div class="stars"> |
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 block with stars should have a modifier class (e.g., 'stars--4') to indicate how many stars should be active. The current implementation with 'stars-4' and 'stars-4__star' seems to be incorrect according to BEM methodology.
<span class="price__number">$2,199</span> | ||
</div> | ||
|
||
<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.
It's recommended to use a button element for actions like 'buy' instead of an anchor tag. Anchor tags are intended for navigation, while buttons are meant for actions.
|
||
.product-card { | ||
display: flex; | ||
box-sizing: border-box; |
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.
According to the Code Style Rules ❗️, box-sizing: border-box;
should be set globally for all elements using the universal selector *
, and not for individual elements. This ensures consistency across the entire project.
height: 134px; | ||
margin-bottom: 40px; | ||
align-self: center; | ||
margin-inline: 3px; |
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 margin-inline
property is not standard and may not work in all browsers. It's better to use standard margin properties like margin-left
and margin-right
for compatibility.
.stars { | ||
display: flex; | ||
justify-content: space-between; | ||
margin-bottom: 24px; | ||
} |
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 .stars
block should have a modifier (like .stars--4
or .stars--5
) to indicate the number of active stars, rather than creating a separate block for .stars-4
. This follows the BEM methodology for modifiers and helps keep the CSS more maintainable and scalable.
.stars-4 { | ||
display: flex; | ||
justify-content: space-between; | ||
width: 96px; | ||
} |
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.
It seems you've created a separate block for .stars-4
, which is not necessary. You can use the .stars
block with a modifier for the number of active stars. For example, .stars--4
to indicate 4 active stars. This follows the BEM methodology and keeps the styles more consistent.
.stars-4__star { | ||
height: 16px; | ||
width: 16px; | ||
justify-content: center; | ||
align-items: center; | ||
background-image: url(../images/star.svg); | ||
background-repeat: no-repeat; | ||
background-position: center; | ||
border-radius: 0.5px; | ||
} |
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 .stars-4__star
should be an element of the .stars
block (e.g., .stars__star
) and use a modifier to indicate the active state (e.g., .stars__star--active
). This is more in line with BEM conventions, where the block is the base and elements are parts of the block, while modifiers indicate different states or versions of the block or element.
.stars-4__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.
Instead of creating a separate selector for active stars, use the BEM modifier convention. For example, .stars__star--active
to indicate an active star. This will keep your CSS consistent with BEM methodology and make it easier to manage.
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 SCSS file shows good adherence to coding standards with room for improvement. Great job on following best practices! Remember to maintain consistency, consider dynamic content, and avoid redundancy. Keep up the good work, and always strive to refine your code! 🌟💡
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -12,7 +12,48 @@ | |||
href="./styles/index.scss" |
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 href
attribute should point to a .css
file, not a .scss
file. SCSS files are preprocessed into CSS, which browsers can understand. Ensure that your build process compiles SCSS to CSS and reference the CSS file here.
<div class="product-card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/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.
According to the Code Style Rules ❗️, using <br>
to break lines in a paragraph is not recommended. Consider wrapping the product name and model in separate elements and controlling their display through CSS.
<div class="stars"> | ||
<div class="stars-4"> | ||
<div class="stars-4__star"></div> | ||
<div class="stars-4__star"></div> | ||
<div class="stars-4__star"></div> | ||
<div class="stars-4__star"></div> | ||
<div class="stars-4__star"></div> | ||
</div> | ||
<span class="reviews">Reviews: 5</span> |
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.
It seems like the stars
block is intended to be a rating component. However, the class naming suggests that it is tied to a specific rating ('stars-4'). Consider using a more general class name and control the number of active stars with a modifier class or through a CSS class that represents the rating dynamically.
<a | ||
class="buy-link" | ||
href="#" | ||
data-qa="hover" | ||
> | ||
<div class="button-buy">buy</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 href
attribute of the anchor tag is set to #
, which is a placeholder. If this link is intended to perform an action (like adding to cart), it should have a more descriptive href
value or be a button element if it doesn't navigate to another page.
.product-card { | ||
display: flex; | ||
box-sizing: border-box; | ||
width: 200px; | ||
background-color: #fff; | ||
border: 1px solid #f3f3f3; | ||
border-radius: 5px; | ||
justify-content: center; | ||
flex-direction: column; | ||
padding: 32px 16px 16px; | ||
} |
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.
According to the Code Style Rules, it's recommended to order CSS properties in a specific sequence. For example, position-related properties should come before display and box-model properties. Please reorder the properties within the .product-card
block accordingly.
.product-card__image { | ||
width: 160px; | ||
height: 134px; | ||
margin-bottom: 40px; | ||
align-self: center; | ||
margin-inline: 3px; | ||
} |
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 margin-inline: 3px;
property on line 27 is not a standard margin property and may not be necessary here, as align-self: center;
is already used to center the image. Please consider removing it if it's not serving a specific purpose.
.stars { | ||
display: flex; | ||
justify-content: space-between; | ||
margin-bottom: 24px; | ||
} |
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 .stars
block has a justify-content: space-between;
property which might not be necessary or could lead to unexpected behavior if more elements are added to the block. If the stars are meant to be spaced evenly, consider using justify-content: space-around;
or a specific margin.
.stars-4 { | ||
display: flex; | ||
justify-content: space-between; | ||
width: 96px; | ||
} |
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 .stars-4
block seems to be a modifier for .stars
, but it also includes display: flex;
and justify-content: space-between;
which are already defined in the .stars
block. To avoid redundancy, you can remove these properties from .stars-4
.
.stars-4__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 use of :nth-child(-n + 4)
is a clever way to apply styles to the first four elements. However, ensure this is flexible enough to accommodate different numbers of active stars if the number of stars needs to be dynamic.
.button-buy:hover { | ||
color: #00acdc; | ||
background-color: #f3f3f3; | ||
border: 1px solid #00acdc; | ||
} |
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.
For the .button-buy:hover
state, the addition of border: 1px solid #00acdc;
changes the button's box model size because it adds a border that wasn't there before. To avoid this, consider adding a transparent border to the default state of the button so that the size doesn't change on hover.
card
andstars
each in it own fileelements
,modifiers
andpseudo-classes
stars--N
modifiers work as expected (Highlight firstN
stars)