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 task solution #4820

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

add task solution #4820

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36e1b35
add task solution
OlhaLishenko Oct 7, 2024
50fc3b3
add task solution
OlhaLishenko Oct 7, 2024
8c92303
add task solution
OlhaLishenko Oct 7, 2024
f97ba31
add task solution
OlhaLishenko Oct 7, 2024
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://OlhaLishenko.github.io/layout_product-cards/)
- [TEST REPORT LINK](https://OlhaLishenko.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
Binary file added src/images/mac.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
53 changes: 52 additions & 1 deletion src/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,59 @@
rel="stylesheet"
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.

According to the Code Style Rules, SCSS files should be linked in HTML as CSS files after they have been compiled. You should include the compiled CSS file instead of the SCSS file directly.

/>
<link
rel="preconnect"
href="https://fonts.googleapis.com"
/>
<link
rel="preconnect"
href="https://fonts.gstatic.com"
/>
<link
href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,400;0,500;0,700;1,400&display=swap"
rel="stylesheet"
/>
</head>
<body>
<h1>Product cards</h1>
<div
class="product-card"
id="product-card"
data-qa="card"
>
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 both class and id with the same name product-card is redundant and can lead to confusion. It's better to use either a class or an id for styling and JavaScript hooks, but not both with the same name. If id is not used for JavaScript, you can remove it.

<div class="product-card__image"></div>

<div class="product-content">
<div class="product-content__name">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>

<div class="product-content__code">Product code: 195434</div>
Copy link

Choose a reason for hiding this comment

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

The class naming convention seems to be BEM, but product-content is not consistent with the parent block product-card. It should be product-card__content to maintain BEM conventions.


<div class="product-content__rating">
<div class="stars">
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>

<div class="product-reviews">Reviews: 5</div>
</div>

<div class="product-content__price">
<div class="text-price text-price--text">Price:</div>
Copy link

Choose a reason for hiding this comment

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

The class text-price--text does not follow the BEM methodology correctly. Modifier classes should not be used on their own; they should modify an existing block or element class. Consider renaming to product-content__price-text or similar.

<div class="text-price text-price--value">$2,199</div>
Copy link

Choose a reason for hiding this comment

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

The class text-price--value is used incorrectly as a modifier without the base class. It should be used along with the base class, or you should rename it to follow the BEM convention, such as product-content__price-value.

</div>

<a
href="#product-card"
class="buy-button"
data-qa="hover"
Copy link

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 used for test hooks, but hover is not descriptive of its purpose. It should describe the element or action it's associated with, such as data-qa="buy-button".

>
Buy
</a>
</div>
</div>
</body>
</html>
136 changes: 136 additions & 0 deletions src/styles/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,139 @@
$border-color: #f3f3f3;
$product-card-width: 200px;
$main-fonts: 'Roboto';
$main-text-color: #060b35;
$second-text-color: #616070;
$button-color: #00acdc;
$border-radius: 5px;

body {
margin: 0;
}

.product-card {
display: flex;
flex-direction: column;
box-sizing: border-box;
width: $product-card-width;
border: 1px solid $border-color;
border-radius: $border-radius;
align-items: center;
justify-content: center;

&__image {
background-image: url(../images/imac.jpeg);
background-repeat: no-repeat;
background-size: 100% 100%;
Copy link

Choose a reason for hiding this comment

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

The background image URL should be quoted (url("../images/imac.jpeg")) to avoid potential issues with special characters or spaces in the file path.

width: 160px;
height: 134px;
margin: 32px 19px 0;
box-sizing: border-box;
}
}

.product-content {
margin: 40px 16px 16px;
font-family: $main-fonts, sans-serif;
Copy link

Choose a reason for hiding this comment

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

You have declared the font-family for '.product-content' and its elements multiple times (lines 36, 39, 47, 90, 98). You should define it once globally for the body or a common parent class unless they differ.


&__name {
font-family: $main-fonts, sans-serif;
font-weight: 500;
font-size: 12px;
line-height: 18px;
color: $main-text-color;
}

&__code {
font-family: $main-fonts, sans-serif;
font-weight: 400;
font-size: 10px;
line-height: 14px;
color: $second-text-color;
margin: 4px 0 0;
}

&__rating {
margin: 16px 0 0;
display: flex;
flex-direction: row;
justify-content: space-between;
align-items: flex-end;
}

&__price {
display: flex;
flex-direction: row;
justify-content: space-between;
margin: 24px 0 0;
}
}

.stars {
flex-direction: row;
display: flex;
gap: 4px;

&__star {
background-image: url(../images/star-active.svg);
width: 16px;
height: 16px;
background-repeat: no-repeat;
background-position: center;

&:last-of-type {
background-image: url(../images/star.svg);
}
}
}

.product-reviews {
font-family: $main-fonts, sans-serif;
font-weight: 400;
font-size: 10px;
line-height: 14px;
color: $main-text-color;
}

.text-price {
font-family: $main-fonts, sans-serif;
line-height: 18px;

&--text {
font-weight: 400;
font-size: 12px;
color: $second-text-color;
}

&--value {
font-weight: 700;
font-size: 16px;
color: $main-text-color;
}
}

.buy-button {
box-sizing: border-box;
display: flex;
justify-content: center;
align-items: center;
background-color: $button-color;
text-decoration: none;
color: #ffff;
Copy link

Choose a reason for hiding this comment

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

The color value should be written in lowercase to maintain consistency as per the code style rules.

font-weight: 700;
font-size: 14px;
line-height: 16px;
margin: 16px 0 0;
border-radius: $border-radius;
border: none;
Copy link

Choose a reason for hiding this comment

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

The border property is set twice consecutively (lines 127 and 128). The second declaration overrides the first one, making the first declaration redundant.

border: 1px solid $button-color;
Copy link

Choose a reason for hiding this comment

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

The border property is declared twice with the same value. Remove the duplicate declaration to clean up the code.

width: 166px;
height: 40px;
text-transform: uppercase;

&:hover {
color: $button-color;
background-color: #ffff;
border: 1px solid $button-color;
box-sizing: border-box;
Copy link

Choose a reason for hiding this comment

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

The box-sizing: border-box; declaration in the :hover state of .buy-button is redundant since it's already defined in the normal state (line 115).

}
}
Loading