-
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
add task solution #4631
base: master
Are you sure you want to change the base?
add task solution #4631
Conversation
labelsavchuk
commented
Aug 3, 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, leave some comments
src/index.html
Outdated
<div class="item__name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</div> | ||
<div class="item__code">Product code: 195434</div> | ||
<div class="review"> | ||
<div class="stars stars--4"> | ||
<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="review__count">Reviews: 5</div> | ||
</div> | ||
<div class="item__price"> | ||
<div class="item__price-header">Price:</div> | ||
<div class="item__price-value">$2,199</div> | ||
</div> | ||
</div> | ||
<a | ||
href="#1" | ||
class="card__btn" | ||
data-qa="hover" | ||
> | ||
Buy | ||
</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.
add empty line between blocks
<img | ||
src="./images/imac.jpeg" | ||
alt="i-mac" | ||
class="card__photo" |
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 more informative alt
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.
not fixed.
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.
src/styles/index.scss
Outdated
background: $btn-color; | ||
|
||
&:hover { | ||
background: #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.
move to variable
src/styles/index.scss
Outdated
&__price { | ||
display: flex; | ||
justify-content: space-between; | ||
line-height: 18px; | ||
margin-bottom: 16px; | ||
color: $secondary; | ||
} | ||
&__price-value { | ||
font-size: 16px; | ||
font-weight: 700; | ||
text-align: right; | ||
color: #060b35; | ||
} | ||
|
||
&__price-header { | ||
font-size: 12px; | ||
text-align: left; | ||
color: #616070; | ||
} |
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.
&__price { | |
display: flex; | |
justify-content: space-between; | |
line-height: 18px; | |
margin-bottom: 16px; | |
color: $secondary; | |
} | |
&__price-value { | |
font-size: 16px; | |
font-weight: 700; | |
text-align: right; | |
color: #060b35; | |
} | |
&__price-header { | |
font-size: 12px; | |
text-align: left; | |
color: #616070; | |
} | |
&__price { | |
display: flex; | |
justify-content: space-between; | |
line-height: 18px; | |
margin-bottom: 16px; | |
color: $secondary; | |
&-value { | |
font-size: 16px; | |
font-weight: 700; | |
text-align: right; | |
color: #060b35; | |
} | |
&-header { | |
font-size: 12px; | |
text-align: left; | |
color: #616070; | |
} | |
} | |
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.
Looks good, but let's improve it a bit! 🥹
<img | ||
src="./images/imac.jpeg" | ||
alt="i-mac" | ||
class="card__photo" |
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.
not fixed.
src/index.html
Outdated
<link | ||
href="https://fonts.googleapis.com/css2?family=Poppins:ital,wght@0,100;0,200;0,300;0,400;0,500;0,600;0,700;0,800;0,900;1,100;1,200;1,300;1,400;1,500;1,600;1,700;1,800;1,900&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" | ||
/> |
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 only the fonts u need. u only use a couple of fonts, but u import them all
src/styles/index.scss
Outdated
@@ -1,3 +1,129 @@ | |||
$bc-image: url(/src/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.
do u need this variable?But why?u used it only once
src/styles/index.scss
Outdated
font-family: Roboto, sans-serif; | ||
font-weight: 400; | ||
font-size: 10px; | ||
line-height: 14px; |
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.
do u need it?
I see that u set the fonts globally on the body tag
font-family: Roboto, sans-serif; | |
font-weight: 400; | |
font-size: 10px; | |
line-height: 14px; |
src/styles/index.scss
Outdated
|
||
box-sizing: border-box; | ||
width: 200px; | ||
border: 1px solid #f3f3f3; |
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 variables for all colors
src/styles/index.scss
Outdated
background-image: url(/src/images/star.svg); | ||
background-position: center; | ||
height: 16px; | ||
margin-right: 4px; | ||
width: 16px; | ||
background-repeat: no-repeat; |
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.
background-image: url(/src/images/star.svg); | |
background-position: center; | |
height: 16px; | |
margin-right: 4px; | |
width: 16px; | |
background-repeat: no-repeat; | |
background: ............; | |
height: 16px; | |
margin-right: 4px; | |
width: 16px; |
u can use just background for it
…ation repeat removed
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 work 👍
But don't forget about the checklist
<img | ||
src="./images/imac.jpeg" | ||
alt="i-mac" | ||
class="card__photo" |
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.
src/index.html
Outdated
<div class="item__name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</div> | ||
|
||
<div class="item__code">Product code: 195434</div> | ||
|
||
<div class="review"> | ||
<div class="stars stars--4"> | ||
<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="review__count">Reviews: 5</div> | ||
</div> | ||
|
||
<div class="item__price"> | ||
<div class="item__price-header">Price:</div> | ||
<div class="item__price-value">$2,199</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.
Use semantic tags where possible instead of only divs
src/styles/index.scss
Outdated
line-height: 14px; | ||
} | ||
|
||
.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.
src/styles/index.scss
Outdated
} | ||
} | ||
|
||
.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.
What item is it?
Names should be descriptive
src/styles/index.scss
Outdated
|
||
&.stars--4 :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.
It should be the same functionality as in the stars
task, if modifier is --3
you should show 3 active stars as well
src/styles/index.scss
Outdated
} | ||
} | ||
|
||
.review { |
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.
…card-info, stars modifier fix
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.
Looks good 👍