-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 #5203
base: master
Are you sure you want to change the base?
add task solution #5203
Conversation
viktoriamyhailiak
commented
Jan 22, 2025
- DEMO LINK
- TEST REPORT LINK
- There 2 BEM blocks cards and stars each in their own file
- SCSS Nesting is used for elements, modifiers, and pseudo-classes
- [x]SCSS Variables are used for main values and placed in a separate file
- all stars--N modifiers work as expected (Highlight first N stars)
- Code follows all the Code Style Rules ❗️
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.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request:
-
SCSS Nesting: The code does not show evidence of SCSS nesting for elements, modifiers, and pseudo-classes. Implementing nesting will help organize styles hierarchically and improve maintainability.
-
Stars Modifiers: The task requires
stars--N
modifiers to highlight the firstN
stars. This functionality is missing and needs to be implemented to meet the task requirements. -
Font-weight Values: There are several instances where
font-weight
is incorrectly specified with units (e.g.,font-weight: 500px;
). These should be corrected to numeric values without units (e.g.,font-weight: 500;
). -
Typo in CSS: There's a typo in
align-items: centers;
. It should be corrected toalign-items: center;
.
Once these issues are resolved, your code will be in great shape! Keep up the great work, and feel free to reach out if you need any help. 🌟
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
src/styles/card.scss
Outdated
} | ||
|
||
.card__description { | ||
font-weight: 500px; |
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 font-weight
property should not have a unit like 'px'. It should be a numeric value, such as 500.
src/styles/card.scss
Outdated
|
||
@include card-inline-margin; | ||
|
||
align-items: centers; |
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 'align-items: centers;'. It should be 'align-items: center;'.
src/styles/card.scss
Outdated
border-color: $button-color; | ||
box-shadow: none; | ||
color: $main-background-color; | ||
font-weight: 700px; |
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 font-weight
property should not have a unit like 'px'. It should be a numeric value, such as 700.
src/styles/page.scss
Outdated
.page__body { | ||
font-family: Roboto, serif; | ||
font-optical-sizing: auto; | ||
font-weight: 400px; |
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 font-weight
property should not have a unit like 'px'. It should be a numeric value, such as 400.
src/styles/price.scss
Outdated
@@ -0,0 +1,10 @@ | |||
.price__discription { | |||
font-weight: 400px; |
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 font-weight
property should not have a unit like 'px'. It should be a numeric value, such as 400.
src/styles/price.scss
Outdated
|
||
.price__value { | ||
font-size: 16px; | ||
font-weight: 700px; |
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 font-weight
property should not have a unit like 'px'. It should be a numeric value, such as 700.
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.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
Something went wrong while processing the review.
[CODE: 3]
If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.
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.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
Something went wrong while processing the review.
[CODE: 3]
If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.
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 making significant progress with your project! 🎉 While there are still a few areas to improve, such as implementing SCSS nesting and correcting class name typos, your efforts are commendable. The stars--N
modifiers need attention to ensure they highlight the first N
stars as required. Additionally, ensure that the <html>
tag is properly closed. These improvements will enhance your code's functionality and maintainability. Keep up the good work, and use this feedback to refine your skills further. Remember, addressing these issues will help you avoid similar problems in the future. Happy coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -1,18 +1,71 @@ | |||
<!doctype html> | |||
<html lang="en"> | |||
<html |
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 <html>
tag is not properly closed. It should be <html>
instead of <html /++2++/>
.
src/index.html
Outdated
</p> | ||
<p class="card__product-code">Product code: 195434</p> | ||
<div class="card__review"> | ||
<div class="stars card__starts"> |
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 card__starts
. It should be card__stars
to match the naming convention and likely intended meaning.
@@ -0,0 +1,86 @@ | |||
.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.
Consider using SCSS nesting for better organization. For example, nest styles for .card__photo
, .card__description
, etc., within the .card
block.
@@ -0,0 +1,86 @@ | |||
.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.
The task requires stars--N
modifiers to highlight the first N
stars. Ensure these modifiers are implemented in your 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.
Please fix this AI comment
src/styles/price.scss
Outdated
@@ -0,0 +1,10 @@ | |||
.price__discription { |
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 .price__discription
. It should be .price__description
to match the intended meaning.
@@ -0,0 +1,22 @@ | |||
.stars { |
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.
Consider using SCSS nesting to organize the styles better. For example, nest the .stars__star
styles within the .stars
block.
@@ -0,0 +1,22 @@ | |||
.stars { |
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 task requires stars--N
modifiers to highlight the first N
stars. Ensure these modifiers are implemented in your 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.
Looks like there is a problem with deploy, please fix it. Feel free to ask for help in fe_chat
src/styles/stars.scss
Outdated
&--1, | ||
&--2, | ||
&--3, | ||
&--4 { | ||
background-image: url(../images/star-active.svg); | ||
} | ||
&--5 { | ||
background-image: url(../images/star.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.
use scss loop to generate selectors.
src/styles/price.scss
Outdated
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 don't need separate file for price as it part of 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.
There are still issues with your deploy link. Try to delete .parcel-cache and dist folders and make deploy once again. Please feel free to ask for help in fe_chat.
src/index.html
Outdated
data-qa="card" | ||
> | ||
<img | ||
src="/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.
src="/src/images/imac.jpeg" | |
src="images/imac.jpeg" |
src/index.html
Outdated
<link | ||
rel="stylesheet" | ||
href="./styles/index.scss" | ||
href="./styles/main.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.
href="./styles/main.scss" | |
href="styles/main.scss" |
src/index.html
Outdated
<div class="stars__star stars__star--1"></div> | ||
<div class="stars__star stars__star--2"></div> | ||
<div class="stars__star stars__star--3"></div> | ||
<div class="stars__star stars__star--4"></div> | ||
<div class="stars__star stars__star--5"></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="stars__star stars__star--1"></div> | |
<div class="stars__star stars__star--2"></div> | |
<div class="stars__star stars__star--3"></div> | |
<div class="stars__star stars__star--4"></div> | |
<div class="stars__star stars__star--5"></div> | |
<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> |
src/index.html
Outdated
</p> | ||
<p class="card__product-code">Product code: 195434</p> | ||
<div class="card__review"> | ||
<div class="stars card__stars"> |
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="stars card__stars"> | |
<div class="stars stars--4"> |
src/index.html
Outdated
</div> | ||
<p class="card__review-amount">Reviews: 5</p> | ||
</div> | ||
<div class="price card__price"> |
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 card__price"> | |
<div class="card__price"> |
src/index.html
Outdated
<p class="price__description">Price:</p> | ||
<p class="price__value">$2,199</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="price__description">Price:</p> | |
<p class="price__value">$2,199</p> | |
<p class="card__price-description">Price:</p> | |
<p class="card__price-value">$2,199</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.
Please fix styles as well
src/index.html
Outdated
<input | ||
type="button" | ||
value="BUY" | ||
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.
It's not necessary to use input here, just add styles for a tag
@@ -0,0 +1,86 @@ | |||
.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.
Please fix this AI comment
background-image: url(../images/star.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.
} | |
} | |
@for $i from 0 through 5 { | |
.stars--#{$i} > .stars__star:nth-child(-n + #{$i}) { | |
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.
Good job.
Almost done. Try to pass tests.
Check vertical sizes for all elements.
Total card height should be 408px as in design.
- make paddings for .card
- don't use horizontal margins for card elements
- for make space betweent elements - use same vertical margin (margin-bottom will be good).
- some tags has default margins (h1, h2... p, ). Reset them