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.

Sign up for GitHub

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

Jump to bottom

add solution #4831

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add solution #4831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ Create a pages with product card using `flexbox`, `BEM` and `SCSS` based on [thi

❗️ Replace `<your_account>` with your Github username and copy the links to `Pull Request` description:

- [DEMO LINK](https://<your_account>.github.io/layout_product-cards/)
- [TEST REPORT LINK](https://<your_account>.github.io/layout_product-cards/report/html_report/)
- [DEMO LINK](https://petro-boiarenko.github.io/layout_product-cards/)
- [TEST REPORT LINK](https://petro-boiarenko.github.io/layout_product-cards/report/html_report/)

❗️ Copy this `Checklist` to the `Pull Request` description after links, and put `- [x]` before each point after you checked it.

Expand Down
45 changes: 43 additions & 2 deletions src/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,48 @@
href="./styles/index.scss"
Copy link

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.

Copy link

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.

Copy link

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.

/>
</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
Copy link

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.

Comment on lines +26 to +29
Copy link

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.

Comment on lines +26 to +29
Copy link

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>

<div class="product-card__code">Product code: 195434</div>

<div class="stars">
Copy link

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.

<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
Copy link

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

<span class="reviews">Reviews: 5</span>
Comment on lines +34 to +42
Copy link

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.

</div>

<div class="price">
<span class="price__text">Price:</span>
<span class="price__number">$2,199</span>
</div>

<a
Copy link

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.

class="buy-link"
href="#"
data-qa="hover"
>
<div class="button-buy">buy</div>
Copy link

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

. This ensures that the button is accessible and semantically correct.

Comment on lines +50 to +55
Copy link

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.

</a>
</div>
</body>
</html>
121 changes: 120 additions & 1 deletion src/styles/index.scss
View file
Open in desktop
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;
Copy link

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.

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
Copy link

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;
}
Copy link

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.

Copy link

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.


Comment on lines +22 to +28
Copy link

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.

.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;
Copy link

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.

}

Comment on lines +48 to +52
Copy link

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.

Comment on lines +48 to +52
Copy link

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;
Copy link

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.

}

Comment on lines +54 to +58
Copy link

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.

Comment on lines +54 to +58
Copy link

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 {
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
Copy link

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);
}
Comment on lines +71 to +72
Copy link

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.


Comment on lines +71 to +73
Copy link

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.

Comment on lines +71 to +73
Copy link

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.

.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
Copy link

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.

.buy-link {
text-decoration: none;
color: inherit;
}
Loading