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 #4821

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

add task solution #4821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
306ecbb
add task solution
dmdamyan Oct 7, 2024
e714143
added corrections
dmdamyan Oct 7, 2024
793ad12
corrected comments
dmdamyan 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://dmdamyan.github.io/layout_product-cards/)
- [TEST REPORT LINK](https://dmdamyan.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/grade.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.
Binary file added src/images/price.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.
Binary file added src/images/star.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.
Binary file added src/images/stars.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.
75 changes: 74 additions & 1 deletion src/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,90 @@
<html lang="en">
<head>
<meta charset="UTF-8" />

<meta
name="viewport"
content="width=device-width, initial-scale=1.0"
/>

<title>Product cards</title>

<link
rel="preconnect"
href="https://fonts.googleapis.com"
/>

<link
rel="preconnect"
href="https://fonts.gstatic.com"
crossorigin="anonymous"
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, attribute crossorigin should be used with the link tag only when it is necessary. In this case, it seems redundant as the Google Fonts API does not require CORS for font serving.

/>

<link
href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap"
rel="stylesheet"
/>

<link
rel="stylesheet"
href="./styles/index.scss"
/>
</head>

<body>
<h1>Product cards</h1>
<section
class="card"
data-qa="card"
>
<div class="card__image">
Copy link

Choose a reason for hiding this comment

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

The BEM naming convention suggests that elements of a block should be named using __ to separate the block from the element. Here, card__image is correct, but the nested img element should follow the same convention and be named something like card__photo instead of card__image-photo.

<img
class="card__image-photo"
src="images/imac.jpeg"
alt="product photo"
/>
</div>

<h2 class="card__model">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h2>

<div class="card__product-code">
<p class="card__product-code-style">Product code: 195434</p>
Copy link

Choose a reason for hiding this comment

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

The p tag with class card__product-code-style should be named according to BEM methodology. Since it is an element of the card block, it should be named card__product-code.

</div>

<div class="card__grade">
<div class="card__grade-stars">
<ul class="card__grade-stars-list">
<li class="card__grade-stars-list-1"></li>
Copy link

Choose a reason for hiding this comment

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

The BEM naming convention suggests that elements should not be named with numbers at the end. Instead, use a modifier if necessary. For example, card__star and card__star--1, card__star--2, etc., if different styles are needed for each star.


<li class="card__grade-stars-list-2"></li>

<li class="card__grade-stars-list-3"></li>

<li class="card__grade-stars-list-4"></li>

<li class="card__grade-stars-list-5"></li>
</ul>
</div>

<div class="card__grade-reviews">Reviews: 5</div>
</div>
Comment on lines +56 to +72
Copy link

Choose a reason for hiding this comment

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

The rating and reviews could be better represented using <figure> and <figcaption> elements to group the star ratings and the review count semantically.


<div class="card__price">
<p class="card__price-style">Price:</p>
Copy link

Choose a reason for hiding this comment

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

Similar to the previous comment, the p tag with class card__price-style should follow the BEM naming convention. It could be named card__price-label to indicate its purpose more clearly.


<p class="card__price-amount-style">$2,199</p>
Copy link

Choose a reason for hiding this comment

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

The p tag with class card__price-amount-style should follow the BEM naming convention. It could be named card__price-amount to be consistent with BEM.

</div>

<div class="card__buy">
<button
class="card__buy-button"
type="button"
data-qa="hover"
>
BUY
</button>
</div>
</section>
</body>
</html>
133 changes: 132 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,134 @@
body {
.card {
display: flex;
flex-direction: column;
align-items: center;
box-sizing: border-box;
margin: 0;
Copy link

Choose a reason for hiding this comment

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

It is better to set box-sizing: border-box; globally to apply it to all elements. Consider adding *, *::before, *::after { box-sizing: border-box; } at the beginning of your stylesheet.

padding: 0;
width: 200px;
height: 408px;
font-family: Roboto, sans-serif;
Comment on lines +8 to +9
Copy link

Choose a reason for hiding this comment

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

Avoid using fixed widths and heights for elements. This can make the design less responsive and harder to maintain. Consider using max-width or min-width for width, and padding or min-height for height, to allow for more flexibility.

font-weight: 400;

&__image {
margin-top: 33px;

&-photo {
max-width: 160px;
max-height: 134px;
object-fit: contain;
}
}
Comment on lines +13 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 BEM methodology, an element should not be nested inside another element. Here, &__image-photo is an element nested inside &__image, which is also an element. Consider refactoring to follow BEM conventions, perhaps by using a modifier or a block instead.


&__model {
margin-top: 40px;
width: 166px;
height: 36px;
font-size: 12px;
Comment on lines +25 to +26
Copy link

Choose a reason for hiding this comment

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

Fixed width and height on text containers can lead to text overflow or clipping issues. Use padding along with min-height or min-width as needed, and ensure text has enough room to scale with different screen sizes or font settings.

font-weight: 500;
padding-top: 0;
color: #060b35;
}

&__product-code {
margin-top: 4px;
align-self: start;
width: 98px;
height: 14px;

&-style {
white-space: nowrap;
padding-left: 17px;
font-size: 10px;
line-height: 14px;
color: #616070;
margin: 0;
}
}

&__grade {
display: flex;
flex-direction: row;
justify-content: space-between;
width: 166px;
height: 16px;
margin-top: 16px;

&-stars {
box-sizing: border-box;
width: 96px;
height: 16px;

&-list {
list-style: none;
justify-content: space-between;
margin: 1px;

&:nth-child(n + 1) {
background-image: url(../images/star.png);
Copy link

Choose a reason for hiding this comment

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

The &:nth-child(n + 1) selector seems redundant here since it applies to all child elements, effectively having no specific selection criteria. You can remove this selector and apply the styles directly to &-list if there are no other children that the styles should not apply to.

width: 14px;
height: 14px;
}

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 display: block; on a li element (which is block-level by default) is unnecessary. If you need to apply specific styles to list items, consider targeting them with a class instead.

&-5 {
visibility: hidden;
}
}
Comment on lines +73 to +75
Copy link

Choose a reason for hiding this comment

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

Setting an element to visibility: hidden will hide it but still take up the same space as if it were visible. If the intent is to not display the fifth star at all, consider using display: none instead. However, if the space needs to be reserved (but the star not shown), visibility: hidden is correct.

}

&-reviews {
min-width: 53px;
height: 16px;
line-height: 16px;
font-size: 10px;
color: #060b35;
}
}

&__price {
display: flex;
justify-content: space-between;
width: 166px;
height: 18px;
margin-top: 24px;

&-style {
display: block;
min-width: 31px;
height: 18px;
font-size: 12px;
line-height: 18px;
margin: 0;
color: #616070;
}

&-amount-style {
margin: 0;
width: 50px;
height: 18px;
font-size: 16px;
font-weight: 700;
line-height: 18px;
color: #060b35;
}
}

&__buy-button {
width: 166px;
height: 40px;
border-radius: 5px;
border-color: inherit;
background-color: #00acdc;
Copy link

Choose a reason for hiding this comment

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

The property border-color: inherit; is unnecessary if you are not setting a specific border style or width. If you want to ensure the button has a border, specify it explicitly with border-style and border-width.

Copy link

Choose a reason for hiding this comment

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

The border-color: inherit; declaration will inherit the border color from the parent element, but since there's no border color set on the parent, this line has no effect. If you want the button to have a border color, you should specify it explicitly.

margin-top: 16px;
font-size: 14px;
Copy link

Choose a reason for hiding this comment

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

The margin-top property is used quite often with the same value of 16px. Consider creating a utility class or a mixin to avoid repetition and make the code DRY (Don't Repeat Yourself).

font-weight: 700;
line-height: 16px;
text-align: center;
color: #fff;

&:hover {
color: #00acdc;
background-color: #fff;
}
}
}
Loading