-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
base: master
Are you sure you want to change the base?
Develop #4964
Conversation
IvanIlnytskyy
commented
Dec 30, 2024
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this 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> |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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="#" |
There was a problem hiding this comment.
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.
src/styles/index.scss
Outdated
&: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; |
There was a problem hiding this comment.
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.
src/styles/index.scss
Outdated
gap: 4px; | ||
margin-right: 4px; | ||
|
||
&:nth-child(-n + 4) { |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all comments
<!-- #region header --> |
src/index.html
Outdated
</a> | ||
<nav class="nav"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</a> | |
<nav class="nav"> | |
</a> | |
<nav class="nav"> |
src/index.html
Outdated
</li> | ||
<li class="nav__item"> |
There was a problem hiding this comment.
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
</li> | |
<li class="nav__item"> | |
</li> | |
<li class="nav__item"> |
src/index.html
Outdated
<div class="mark__stars"> | ||
<span class="star"></span> | ||
<span class="star"></span> | ||
<span class="star"></span> | ||
<span class="star"></span> | ||
<span class="star"></span> |
There was a problem hiding this comment.
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
<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/styles/index.scss
Outdated
display: flex; | ||
align-items: center; | ||
justify-content: space-between; | ||
background-color: #fff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This color is default
background-color: #fff; |
src/styles/index.scss
Outdated
} | ||
|
||
$hover-text-color: #34568b; | ||
$color: #00acdc; |
There was a problem hiding this comment.
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
$color: #00acdc; | |
$main-color: #00acdc; |
src/styles/index.scss
Outdated
.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); | ||
} |
There was a problem hiding this comment.
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
src/styles/index.scss
Outdated
.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); | ||
} | ||
} |
There was a problem hiding this comment.
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
src/styles/index.scss
Outdated
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; | ||
} |
There was a problem hiding this comment.
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
41aeba2
to
8e8723c
Compare
There was a problem hiding this 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"> |
There was a problem hiding this comment.
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
<div class="product-cards"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</header> | ||
<main class="catalog"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</header> | |
<main class="catalog"> | |
</header> | |
<main class="catalog"> |
src/index.html
Outdated
</div> | ||
<div |
There was a problem hiding this comment.
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
</div> | |
<div | |
</div> | |
<div |
src/index.html
Outdated
</header> | ||
<main class="catalog"> | ||
<div | ||
class="product-card" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class="product-card" | |
class="card" |
src/index.html
Outdated
<img | ||
src="./images/imac.jpeg" | ||
alt="imac" | ||
class="product-card__image" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class="product-card__image" | |
class="card__image" |
src/index.html
Outdated
alt="imac" | ||
class="product-card__image" | ||
/> | ||
<h2 class="product-card__title"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div class="mark"> | |
<div class="card__review"> |
src/index.html
Outdated
<div class="price"> | ||
<p class="price__word">Price:</p> | ||
<p class="price__numeric">$2,199</p> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class="bottom" | |
class="card__button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 🔥