-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 #4845
base: master
Are you sure you want to change the base?
add task solution #4845
Conversation
alena-bondar
commented
Sep 20, 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.
It looks like you've made a great effort on this Pull Request! However, there are a few important adjustments to make before it's ready to merge. Please address the feedback regarding fallback fonts, redundant CSS, the use of the *
selector, attribute formatting, and consistent indentation. These changes will ensure your code is more maintainable and adheres to standard practices. Keep up the good work, and I'm sure your code will be even better with these improvements! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<body> | ||
<h1>Stars</h1> | ||
<div class="stars stars--0"> | ||
<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> | ||
</div> | ||
|
||
<div class="stars stars--1"> | ||
<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> | ||
</div> | ||
|
||
<div class="stars stars--2"> | ||
<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> | ||
</div> | ||
|
||
<div class="stars stars--3"> | ||
<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> | ||
</div> | ||
|
||
<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> | ||
</div> | ||
|
||
<div class="stars stars--5"> | ||
<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> | ||
</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 empty lines between multiline sibling blocks of HTML to simplify reading the code. However, make sure not to add them between parent and child elements. For example, there should be an empty line between the closing </div>
tag of one block and the opening <div>
tag of the next block.
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.
I already have an empty line between the closing tag of one block and the opening
src/style.css
Outdated
* { | ||
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 the *
selector as it impacts performance. Instead, set styles only for elements that require them.
src/style.css
Outdated
box-sizing: border-box; | ||
} | ||
|
||
body { |
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 body
tag is an exception where you can use tag names for styling, so this is acceptable. However, since you have already reset the margin in the *
selector, this is redundant and can be removed.
justify-content: flex-start; | ||
} | ||
|
||
.stars__star { |
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.
Remember to use fallback fonts - alternative font-family in case the main one doesn't work. Here you could specify a fallback font-family for .stars__star
if you have a custom font applied.
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 Pull Request! Just a few minor suggestions to enhance your code's readability and maintainability. Remember to use fallback fonts to ensure text is always displayed nicely, keep lines under 80 characters for better readability, and maintain consistent indentation to make the structure of your HTML clear. These aren't blockers but following these practices will make your code even better! Keep up the good work! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<body> | ||
<h1>Stars</h1> | ||
<div class="stars stars--0"> | ||
<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> | ||
</div> | ||
|
||
<div class="stars stars--1"> | ||
<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> | ||
</div> | ||
|
||
<div class="stars stars--2"> | ||
<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> | ||
</div> | ||
|
||
<div class="stars stars--3"> | ||
<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> | ||
</div> | ||
|
||
<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> | ||
</div> | ||
|
||
<div class="stars stars--5"> | ||
<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> | ||
</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.
The tag and its children should have a 2-space indentation to reflect the nesting level properly.
} | ||
|
||
.stars__star { | ||
font-family: Arial, sans-serif; |
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.
Remember to use fallback fonts - alternative font-family in case the main one doesn't work. It's a good practice to specify a generic family as a fallback. For example, font-family: Arial, 'Helvetica Neue', Helvetica, sans-serif;
ensures that if Arial is not available, the browser will try Helvetica Neue, then Helvetica, and finally any available sans-serif font.