-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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 #4942
base: master
Are you sure you want to change the base?
add task solution #4942
Conversation
LiliiaVol
commented
Oct 15, 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 HTML structure! 👍 However, there are a few improvements needed in your CSS file. Try to avoid code repetition and be mindful of where you apply global styles. Keep going! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/style.css
Outdated
body { | ||
margin: 0; | ||
padding: 0; | ||
box-sizing: border-box; | ||
} |
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 use tag names for styling (except html
and body
). Although this is an exception, it's better to avoid setting global styles such as margin
, padding
, and box-sizing
on the body
tag. It's more common to set these on the *
selector. But, as per the checklist, using *
selector is not recommended due to its impact on performance. So, it's better to set these properties on individual elements as needed.
src/style.css
Outdated
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--3 .stars__star:nth-child(1) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--3 .stars__star:nth-child(2) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--3 .stars__star:nth-child(3) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--4 .stars__star:nth-child(1) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--4 .stars__star:nth-child(2) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--4 .stars__star:nth-child(3) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--4 .stars__star:nth-child(4) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--5 .stars__star:nth-child(1) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--5 .stars__star:nth-child(2) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--5 .stars__star:nth-child(3) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--5 .stars__star:nth-child(4) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
background-repeat: no-repeat; | ||
} | ||
|
||
.stars--5 .stars__star:nth-child(5) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
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.
The styles for .stars__star
are repeated multiple times for different parent classes .stars--1
, .stars--2
, etc. This repetition can be avoided by using the :nth-child(-n+X)
pseudo-class, where X is the number of active stars. For example, .stars--1 .stars__star:nth-child(-n+1)
will select the first child of the parent .stars--1
, .stars--2 .stars__star:nth-child(-n+2)
will select the first two children of the parent .stars--2
, and so on. This way, you will avoid repeating the same styles and your CSS will be much cleaner and easier to maintain.
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 your code! There are no critical issues present, but there are areas for improvement. You are repeating styles for each '.stars--n .stars__star:nth-child(-n + n)' selector. Consider using a common class for the active stars and apply the styles to this class. Also, try to use semantic tags where possible instead of div
for better SEO and accessibility. Keep learning and improving! 👍🏼
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -13,6 +13,54 @@ | |||
/> | |||
</head> | |||
<body> | |||
<h1>Stars</h1> | |||
<div class="container"> |
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 would be better to use semantic tags where possible. For example, instead of using a div
with class container
, you could use a section
or main
tag. This will improve your page SEO and help screen readers. Remember, div
does not have any meaning.
<div class="stars stars--0"> | ||
<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> |
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 adding some meaningful content inside these div
tags. If these are just placeholders for future content, it's ok, but if not, they should contain some content. Empty elements can lead to confusion and may not be accessible.
<div class="stars stars--0"> | ||
<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="stars stars--1"> | ||
<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="stars stars--2"> | ||
<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="stars stars--3"> | ||
<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="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="stars stars--5"> | ||
<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> |
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 adding some comments or labels to describe what these elements are for. It's not clear what the purpose of these div
elements with the class stars
and stars__star
is.
margin: 0; | ||
padding: 0; | ||
box-sizing: border-box; | ||
} |
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 are using the '*' selector here. This can impact performance as this selector applies styles to every single element in the page. It's better to apply styles only to the elements that require them.
.stars--1 .stars__star:nth-child(1) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
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.
You are repeating styles for each '.stars--n .stars__star:nth-child(-n + n)' selector. This is not efficient. Try to avoid repeating styles as much as possible. You can create a common class for the active stars and apply the styles to this class instead.
.stars--2 .stars__star:nth-child(-n + 2) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
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.
Same issue here: you are repeating styles for each '.stars--n .stars__star:nth-child(-n + n)' selector. Use a common class for the active stars and apply the styles to this class.
.stars--3 .stars__star:nth-child(-n + 3) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
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.
Same issue here: you are repeating styles for each '.stars--n .stars__star:nth-child(-n + n)' selector. Use a common class for the active stars and apply the styles to this class.
.stars--4 .stars__star:nth-child(-n + 4) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
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.
Same issue here: you are repeating styles for each '.stars--n .stars__star:nth-child(-n + n)' selector. Use a common class for the active stars and apply the styles to this class.
.stars--5 .stars__star:nth-child(-n + 5) { | ||
margin-right: 4px; | ||
width: 16px; | ||
height: 16px; | ||
background-image: url(images/star-active.svg); | ||
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.
Same issue here: you are repeating styles for each '.stars--n .stars__star:nth-child(-n + n)' selector. Use a common class for the active stars and apply the styles to this class.