-
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 #4875
add task solution #4875
Conversation
Welbrn
commented
Oct 16, 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 the pull request! 🎉 While there are a few minor typos and a small issue with the stylesheet link, they don't critically impact the functionality. Keep up the good work and pay attention to these details next time! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<link | ||
href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;500&display=swap" | ||
rel="stylesheet" | ||
/> | ||
<link | ||
rel="stylesheet" | ||
href="./styles/index.scss" |
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 stylesheet link should point to a CSS file, not an SCSS file. SCSS files need to be compiled to CSS before they can be linked in HTML.
> | ||
<img | ||
src="/src/images/imac.jpeg" | ||
alt="prodact img" |
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.
There is a typo in the alt attribute. It should be 'product img' instead of 'prodact img'.
src/index.html
Outdated
<span class="product-card__star"></span> | ||
<span class="product-card__star"></span> | ||
|
||
<span class="product-card__reviws-count">Reviews: 5</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.
There is a typo in the class name 'product-card__reviws-count'. It should be 'product-card__reviews-count'.
src/styles/index.scss
Outdated
background-image: url(/src/images/star.svg); | ||
} | ||
|
||
&__reviws-count { |
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.
There is a typo in the class name &__reviws-count
. It should be &__reviews-count
to correctly reflect the word 'reviews'.
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.
> | ||
<img | ||
src="/src/images/imac.jpeg" | ||
alt="prodact img" |
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.
<span class="card__price-value">$2,199</span> | ||
</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.
This div looks redundant
.card { | ||
width: 200px; | ||
|
||
@include flex('', ''); |
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.
Why do you path two empty strings to flex mixin? It looks strange
} | ||
|
||
&__product-title { | ||
margin: 0 16px; |
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 should set padding to the card instead, so it would be easier to control.
As for now all of your tests fail, and that's one of the reasons
@include flex(center, ''); | ||
@include font(14px, 700, 16px); | ||
|
||
width: 166px; |
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.
Don't hardcode width, it should fit all space
width: 166px; | ||
height: 40px; | ||
margin: 16px; | ||
padding: 12px 70px 12px 69px; |
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.
If would be more text in the button it wouldn't fit
margin-right: 4px; | ||
} | ||
|
||
&.stars--4 .stars__star: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 as in the stars task, if the modifier is --3
you should show 3 active stars, if --4
- four active stars, and so on
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 and variables files are duplicated