-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,48 @@ | |
href="./styles/index.scss" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
/> | ||
</head> | ||
<body> | ||
<h1>Product cards</h1> | ||
<body class="page"> | ||
<div | ||
class="product-card" | ||
data-qa="card" | ||
> | ||
<img | ||
src="./images/imac.jpeg" | ||
class="product-card__image" | ||
alt="imac" | ||
/> | ||
|
||
<div class="product-card__title"> | ||
APPLE A1419 iMac 27" Retina | ||
<br /> | ||
5K Monoblock (MNED2UA/A) | ||
Comment on lines
+26
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using
Comment on lines
+26
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using
Comment on lines
+26
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the Code Style Rules ❗️, using |
||
</div> | ||
|
||
<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 commentThe 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. |
||
<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> | ||
Comment on lines
+34
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'. |
||
<span class="reviews">Reviews: 5</span> | ||
Comment on lines
+34
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the |
||
</div> | ||
|
||
<div class="price"> | ||
<span class="price__text">Price:</span> | ||
<span class="price__number">$2,199</span> | ||
</div> | ||
|
||
<a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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 commentThe 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 . This ensures that the button is accessible and semantically correct.
Comment on lines
+50
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
</a> | ||
</div> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,122 @@ | ||
body { | ||
.page { | ||
margin: 0; | ||
padding: 0; | ||
font-family: Roboto, sans-serif; | ||
color: #060b35; | ||
|
||
scroll-behavior: smooth; | ||
} | ||
|
||
.product-card { | ||
display: flex; | ||
box-sizing: border-box; | ||
width: 200px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the Code Style Rules ❗️, |
||
background-color: #fff; | ||
border: 1px solid #f3f3f3; | ||
border-radius: 5px; | ||
justify-content: center; | ||
flex-direction: column; | ||
padding: 32px 16px 16px; | ||
} | ||
|
||
Comment on lines
+10
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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__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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
||
Comment on lines
+22
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
.product-card__title { | ||
text-align: left; | ||
margin-bottom: 4px; | ||
font-size: 12px; | ||
font-weight: 500; | ||
line-height: 18px; | ||
} | ||
|
||
.product-card__code { | ||
text-align: left; | ||
font-size: 10px; | ||
color: #616070; | ||
|
||
font-weight: 400; | ||
line-height: 14px; | ||
margin-bottom: 16px; | ||
} | ||
|
||
.stars { | ||
display: flex; | ||
justify-content: space-between; | ||
margin-bottom: 24px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
Comment on lines
+48
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Comment on lines
+48
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
.stars-4 { | ||
display: flex; | ||
justify-content: space-between; | ||
width: 96px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
Comment on lines
+54
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems you've created a separate block for
Comment on lines
+54
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
.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; | ||
} | ||
|
||
Comment on lines
+60
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
.stars-4__star:nth-child(-n + 4) { | ||
background-image: url(../images/star-active.svg); | ||
} | ||
Comment on lines
+71
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
Comment on lines
+71
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,
Comment on lines
+71
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of |
||
.reviews { | ||
font-size: 10px; | ||
font-weight: 400; | ||
line-height: 16px; | ||
} | ||
|
||
.price { | ||
display: flex; | ||
justify-content: space-between; | ||
height: 18px; | ||
margin-bottom: 16px; | ||
line-height: 18px; | ||
} | ||
|
||
.price__text { | ||
font-size: 12px; | ||
font-weight: 400; | ||
color: #616070; | ||
} | ||
|
||
.price__number { | ||
font-size: 16px; | ||
font-weight: 700; | ||
} | ||
|
||
.button-buy { | ||
text-transform: uppercase; | ||
text-align: center; | ||
font-size: 14px; | ||
line-height: 40px; | ||
font-weight: 700; | ||
color: #fff; | ||
|
||
background-color: #00acdc; | ||
border-radius: 5px; | ||
height: 40px; | ||
} | ||
|
||
.button-buy:hover { | ||
color: #00acdc; | ||
background-color: #f3f3f3; | ||
border: 1px solid #00acdc; | ||
} | ||
|
||
Comment on lines
+113
to
+117
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the |
||
.buy-link { | ||
text-decoration: none; | ||
color: 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.
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.