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.

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

loyout_product_card_reviev #4816

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 29 additions & 0 deletions .github/workflows/test.yml-template
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
name: Test

on:
pull_request:
branches: [ master ]

jobs:
build:

runs-on: ubuntu-latest

strategy:
matrix:
node-version: [20.x]

steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node-version }}
- run: npm install
- run: npm test
- name: Upload HTML report(backstop data)
if: ${{ always() }}
uses: actions/upload-artifact@v2
with:
name: report
path: backstop_data
2,440 changes: 1,733 additions & 707 deletions package-lock.json

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,12 @@
"@mate-academy/backstop-config": "latest",
"@mate-academy/bemlint": "latest",
"@mate-academy/linthtml-config": "latest",
"@mate-academy/scripts": "^1.8.6",
"@mate-academy/scripts": "^1.9.12",
"@mate-academy/stylelint-config": "latest",
"@parcel/transformer-sass": "^2.12.0",
"backstopjs": "6.3.23",
"jest": "^29.7.0",
"mate-scripts": "^0.0.1",
"parcel": "^2.12.0",
"prettier": "^3.3.2",
"stylelint": "^16.7.0",
Expand Down
4 changes: 2 additions & 2 deletions readme.md
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://nikalaiii.github.io/layout_product-cards/)
- [TEST REPORT LINK](https://nikalaiii.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
61 changes: 59 additions & 2 deletions src/index.html
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<!-- #region head -->
<!doctype html>
<html lang="en">
Comment on lines 2 to 3

Choose a reason for hiding this comment

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

The <!DOCTYPE html> declaration should be in lowercase to adhere to the HTML5 specification and maintain consistency with the code style guide.

<head>
Expand All @@ -6,13 +7,69 @@
name="viewport"
content="width=device-width, initial-scale=1.0"
/>
<link
rel="preconnect"
href="https://fonts.googleapis.com"
/>
<link
rel="preconnect"
href="https://fonts.gstatic.com"

Choose a reason for hiding this comment

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

The preconnect to fonts.gstatic.com is unnecessary if you're using fonts.googleapis.com, as the latter should implicitly handle the connection to the font service.

/>
<link
href="https://fonts.googleapis.com/css2?family=Roboto:wght@100;400;500;700&display=swap"
rel="stylesheet"
/>
<title>Product cards</title>
<link
rel="stylesheet"
href="./styles/index.scss"
href="./styles/styles.css"

Choose a reason for hiding this comment

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

According to the checklist, the project should not contain unused files. If 'styles.css' is a compiled file from SCSS, it should not be included directly in the HTML, but rather served through a build process to ensure the latest styles are applied.

/>
</head>
<!-- #endregion -->
<body>
<h1>Product cards</h1>
<div
class="card"
data-qa="card"
>
<div class="card__content">

Choose a reason for hiding this comment

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

The div element with class card__content could be replaced with a more semantic element such as article or section to better represent the content within a card.

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

<div class="card__title">
<p class="card__title-text">

Choose a reason for hiding this comment

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

The p element inside card__title is unnecessary if it only contains text. You can apply the card__title-text class directly to the div element to reduce HTML complexity.

APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</p>
</div>

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

Choose a reason for hiding this comment

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

Similarly, the p element inside card__code is unnecessary if it only contains text. Consider applying the card__code-text class directly to the div element.

</div>

<div class="card__rating">
<div class="card__rating-stars">

Choose a reason for hiding this comment

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

The rating stars could be implemented using a list (ul) with list items (li) for better semantic meaning and accessibility.

<div class="card__rating-star"></div>
<div class="card__rating-star"></div>
<div class="card__rating-star"></div>
<div class="card__rating-star"></div>
<div class="card__rating-star"></div>
</div>
<p class="card__rating-text">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__price-text">Price:</p>
<p class="card__price-value">$2,199</p>
</div>

<div class="card__buy">
<a

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 a button element or an a element with a valid href attribute pointing to the purchase page or action, not just a # placeholder.

href="#"
class="card__buy-text"
data-qa="hover"
>
Buy
</a>
</div>
</div>
</div>
</body>
</html>
151 changes: 151 additions & 0 deletions src/styles/index.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,154 @@
@import './variables';

// Міксин для центрованих елементів
@mixin flex-center($width, $height) {
display: flex;
justify-content: center;
align-items: center;
width: $width;
height: $height;
}
Comment on lines +4 to +10

Choose a reason for hiding this comment

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

Consider using more descriptive mixin names. The name flex-center is not fully descriptive of what the mixin does, as it also sets the width and height. A more descriptive name could be flex-center-with-size to reflect all its responsibilities.


@mixin star-position {
width: 16px;
height: 16px;
margin-left: 4px;
background-position: center;
background-repeat: no-repeat;
}

// Міксин для стилю тексту
@mixin text-style($font-size, $font-weight, $color) {
font-size: $font-size;
font-weight: $font-weight;
color: $color;
}

html {
font-family: $font-family;
}
Comment on lines +27 to +29

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, the base font size should be set in the html selector using px units. Make sure that the $font-family variable is defined with a fallback system font, e.g., Helvetica, Arial, sans-serif.


body {
margin: 0;
}

p {
margin: 0;
}

.card {
display: flex;
width: $card-width;
height: $card-height;

&__content {

Choose a reason for hiding this comment

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

Avoid setting the same width and height for an element and its BEM element. If .card has a set width and height, the &__content should not repeat those properties unless it is intentionally different.

width: $card-width;
height: $card-height;
border-radius: 5px;
}
Comment on lines +39 to +48

Choose a reason for hiding this comment

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

The &__content block has width and height properties that are the same as the parent .card block. This is redundant and can be removed unless the .card__content is supposed to have different dimensions or is expected to change independently from .card.


&__image {
@include flex-center(160px, 134px);

background-image: url(../images/imac.jpeg);
margin-left: 19px;
margin-top: 32px;
}

Choose a reason for hiding this comment

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

The background-position, background-size, and background-repeat properties are redundant here since they are already included in the flex-center mixin. It's better to keep the code DRY (Don't Repeat Yourself) by not repeating these properties.

Comment on lines +50 to +56

Choose a reason for hiding this comment

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

The &__image block uses fixed pixel values for width, height, and margins. Consider using variables for these values to make the code more maintainable and to ensure consistency across the project.


&__title {
margin-top: 40px;
margin-left: $left-margin;
height: 36px;
width: 166px;

&-text {
@include text-style($text-size, 500, $text-color);

line-height: 18px;
}
Comment on lines +58 to +68

Choose a reason for hiding this comment

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

The &__title block and its child &-text also have fixed pixel values. To maintain consistency and make it easier to change values project-wide, consider using variables for dimensions and margins.

}

&__code {
width: 98px;
height: 14px;
margin-top: 4px;
margin-left: $left-margin;
line-height: 14px;

&-text {
font-size: 10px;
font-weight: 400;
color: $subtext-color;
}
Comment on lines +71 to +82

Choose a reason for hiding this comment

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

The &__code block and its child &-text have fixed pixel values for dimensions, which should be replaced with variables for the same reasons mentioned previously.

}

&__rating {
@include flex-center(166px, 16px);

margin-left: 17px;
margin-top: 16px;

// Цикл для стилізації зірок (діти класу &__rating)
.rating-star {
background-image: url(../images/star.svg); // Зображення за замовчуванням для всіх зірок

// Цикл для стилізації активних зірок
@for $i from 1 through $stars-count {
&:nth-child(#{$i}) {
@if $i <= $active-stars {
background-image: url(../images/star-active.svg); // Заповнена зірка для активних
}

@include star-position; // Позиціонування зірки
}
}
Comment on lines +96 to +104

Choose a reason for hiding this comment

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

The @for loop inside .rating-star is a good use of SCSS features. However, ensure that the $stars-count and $active-stars variables are defined in your variables file and are used consistently throughout your project.

}
}

&-text {
font-size: 10px;
line-height: 14px;
text-align: right;
margin-left: auto;
color: $text-color;
}
}

&__price {
@include flex-center(166px, 16px);

margin-top: 24px;
margin-left: $left-margin;

&-text {
@include text-style($text-size, 400, $subtext-color);
}

&-value {
font-size: 16px;
font-weight: 700;
margin-left: auto;
color: $text-color;
}
Comment on lines +117 to +132

Choose a reason for hiding this comment

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

The &__price block and its children have fixed pixel values and should use variables to maintain consistency and flexibility. Additionally, ensure that the color values are also defined as variables.

}

&__buy {
@include flex-center(166px, 40px);

margin-top: 16px;
margin-left: $left-margin;
background-color: $button-color;
border: 1px solid $button-color;
border-radius: $border-radius;

&-text {
@include text-style($buy-button-font-size, $buy-button-font-weight, white);

text-transform: uppercase;
text-decoration: none;
}

&:hover {
background-color: white;
}
Comment on lines +135 to +153

Choose a reason for hiding this comment

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

The &__buy block correctly uses variables for colors, but ensure that dimensions are also managed through variables. Also, consider defining a variable for the hover state background color to maintain consistency and ease of updates.

}
Loading
Loading