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

Develop #4964

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

Develop #4964

wants to merge 7 commits into from

Conversation

IvanIlnytskyy
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Great job on meeting most of the task requirements! 🎉 While there are a few areas for improvement, such as ensuring unique content for SEO, addressing redundant styles, and adding consistency in class attributes, these are minor issues that don't critically impact the functionality. Keep up the good work and consider these suggestions for future enhancements! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Photo
</a>
</li>
<li>

Choose a reason for hiding this comment

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

The last list item in the navigation does not have a class attribute like the others. Consider adding a class for consistency and easier styling.

src/index.html Outdated
data-qa="card"
>
<img
src="./images/imac.jpeg"

Choose a reason for hiding this comment

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

The image source for the product card is repeated multiple times. If these are meant to be different products, ensure each has a unique image and description.

src/index.html Outdated
/>
<div class="content">
<h2 class="content__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)

Choose a reason for hiding this comment

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

The product title and description are repeated for each product card. Consider using unique content for each card to improve SEO and user engagement.

src/index.html Outdated
<div class="price__numeric">$2,199</div>
</div>
<a
href="#"

Choose a reason for hiding this comment

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

The href attribute for the 'BUY' button is set to '#', which is a placeholder. Ensure this links to the appropriate product page or action.

Comment on lines 220 to 233
&:hover {
display: block;
text-align: center;
padding: 12px;
font-size: 14px;
line-height: 16px;
text-decoration: none;
border-radius: 5px;
font-weight: 700;
position: relative;
background-color: $white;
color: $color;
box-sizing: border-box;
border: 1px solid $color;

Choose a reason for hiding this comment

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

The styles within the &:hover block for .bottom are mostly redundant with the styles defined outside of it. Consider removing the redundant properties to simplify the code.

gap: 4px;
margin-right: 4px;

&:nth-child(-n + 4) {

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) selector is correct for styling the first four stars as active. Ensure that this logic aligns with the intended design for the star rating system.

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Good job 👍
Let's improve your code

src/index.html Outdated
@@ -22,6 +22,389 @@
</head>

<body>
<h1>Catalog</h1>
<!-- #region header -->

Choose a reason for hiding this comment

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

Remove all comments

Suggested change
<!-- #region header -->

src/index.html Outdated
Comment on lines 35 to 36
</a>
<nav class="nav">

Choose a reason for hiding this comment

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

Suggested change
</a>
<nav class="nav">
</a>
<nav class="nav">

src/index.html Outdated
Comment on lines 45 to 47
</li>
<li class="nav__item">

Choose a reason for hiding this comment

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

Add the blank line between the links

Suggested change
</li>
<li class="nav__item">
</li>
<li class="nav__item">

src/index.html Outdated
Comment on lines 126 to 131
<div class="mark__stars">
<span class="star"></span>
<span class="star"></span>
<span class="star"></span>
<span class="star"></span>
<span class="star"></span>

Choose a reason for hiding this comment

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

You need to use this block the previous tasks, fix classes everywhere and add the modifier class

Suggested change
<div class="mark__stars">
<span class="star"></span>
<span class="star"></span>
<span class="star"></span>
<span class="star"></span>
<span class="star"></span>
<div class="stars stars--4">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>

src/index.html Outdated Show resolved Hide resolved
display: flex;
align-items: center;
justify-content: space-between;
background-color: #fff;

Choose a reason for hiding this comment

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

This color is default

Suggested change
background-color: #fff;

}

$hover-text-color: #34568b;
$color: #00acdc;

Choose a reason for hiding this comment

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

Use a more descriptive name for this color, fix it everywhere

Suggested change
$color: #00acdc;
$main-color: #00acdc;

Comment on lines 84 to 33
.catalog {
display: grid;
gap: 46px 48px;
padding: 50px 40px;
align-items: center;
justify-content: center;

@media (min-width: 488px) {
grid-template-columns: repeat(2, 200px);
}

@media (min-width: 768px) {
grid-template-columns: repeat(3, 200px);
}

@media (min-width: 1024px) {
grid-template-columns: repeat(4, 200px);
}

Choose a reason for hiding this comment

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

Move these styles to the separate file

Comment on lines 169 to 182
.star {
background-image: url(../images/star.svg);
background-position: 50%;
background-repeat: no-repeat;
width: 16px;
height: 16px;
border-radius: 0.5px;
gap: 4px;
margin-right: 4px;

&:nth-child(-n + 4) {
background-image: url(../images/star-active.svg);
}
}

Choose a reason for hiding this comment

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

Move stars styles to the separate file

Comment on lines 25 to 51
font-weight: 500;
padding: 0 50px;
box-sizing: border-box;
}

.nav__list {
display: flex;
margin: 0;
padding: 0;
list-style: none;
}

.nav__item {
margin-right: 20px;
}

.nav__link {
text-decoration: none;
color: $text-color;
line-height: 60px;
font-family: Roboto, sans-serif;
font-weight: 500;
text-transform: uppercase;
display: block;
text-align: center;
font-size: 12px;
}

Choose a reason for hiding this comment

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

Move header styles to the separate file

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Hi, you need to fix all comments from the previous review and make a deploy again after your changes

src/index.html Outdated
</a>
</div>
</div>
<div class="product-cards">

Choose a reason for hiding this comment

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

Redundant wrapper, remove it everywhere

Suggested change
<div class="product-cards">

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Hi, the demo link isn't working, you need to make deploy again and check after demo
image

Comment on lines +112 to +113
</header>
<main class="catalog">

Choose a reason for hiding this comment

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

Suggested change
</header>
<main class="catalog">
</header>
<main class="catalog">

src/index.html Outdated
Comment on lines 219 to 220
</div>
<div

Choose a reason for hiding this comment

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

Add the blank line between the all cards

Suggested change
</div>
<div
</div>
<div

src/index.html Outdated
</header>
<main class="catalog">
<div
class="product-card"

Choose a reason for hiding this comment

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

Suggested change
class="product-card"
class="card"

src/index.html Outdated
<img
src="./images/imac.jpeg"
alt="imac"
class="product-card__image"

Choose a reason for hiding this comment

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

Suggested change
class="product-card__image"
class="card__image"

src/index.html Outdated
alt="imac"
class="product-card__image"
/>
<h2 class="product-card__title">

Choose a reason for hiding this comment

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

Suggested change
<h2 class="product-card__title">
<h2 class="card__title">

src/index.html Outdated
<h2 class="product-card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h2>
<p class="product-card__description">Product code: 195434</p>

Choose a reason for hiding this comment

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

Suggested change
<p class="product-card__description">Product code: 195434</p>
<p class="card__description">Product code: 195434</p>

src/index.html Outdated
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h2>
<p class="product-card__description">Product code: 195434</p>
<div class="mark">

Choose a reason for hiding this comment

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

Suggested change
<div class="mark">
<div class="card__review">

src/index.html Outdated
Comment on lines 137 to 140
<div class="price">
<p class="price__word">Price:</p>
<p class="price__numeric">$2,199</p>
</div>

Choose a reason for hiding this comment

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

Suggested change
<div class="price">
<p class="price__word">Price:</p>
<p class="price__numeric">$2,199</p>
</div>
<div class="card__price">
<p class="card__price-label">Price:</p>
<p class="card__price-value">$2,199</p>
</div>

src/index.html Outdated
<a
href="#"
data-qa="card-hover"
class="bottom"

Choose a reason for hiding this comment

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

Suggested change
class="bottom"
class="card__button"

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Great work! 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants