-
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
layout stars for reviev AI #4884
base: master
Are you sure you want to change the base?
Conversation
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.
While you've done a great job with the HTML structure, there are a few formatting issues that we need to tidy up to ensure our code is clean and adheres to best practices. 😊 Remember to avoid using the *
selector for performance reasons, replace tag names with classes where applicable, and keep an eye on those line lengths and indentation for better readability. Let's make these adjustments and you'll be all set! Keep up the good work! 🌟
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -13,6 +13,59 @@ | |||
/> |
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.
Keep your attributes correctly formatted. Since the link element has more than 2 attributes, put each attribute on a new line with proper indentation.
src/index.html
Outdated
<div class="stars stars--0"> | ||
<ul> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> | ||
</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. There should be an empty line between the closing tag of one block and the opening
src/index.html
Outdated
<div class="stars stars--1"> | ||
<ul> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> | ||
</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. There should be an empty line between the closing tag of one block and the opening
src/index.html
Outdated
<div class="stars stars--2"> | ||
<ul> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> | ||
</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. There should be an empty line between the closing tag of one block and the opening
src/index.html
Outdated
<div class="stars stars--3"> | ||
<ul> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> | ||
</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. There should be an empty line between the closing tag of one block and the opening
src/index.html
Outdated
<div class="stars stars--4"> | ||
<ul> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> | ||
</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. There should be an empty line between the closing tag of one block and the opening
src/style.css
Outdated
* { | ||
margin: 0; | ||
padding: 0; | ||
} |
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 *
selector (it impacts performance). Set styles only for elements that require them.
.stars { | ||
margin: 0; | ||
} |
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 recommended not to use tag names for styling (except html
and body
). Style elements using .classes
and if needed with :pseudo-classes
, pseudo-elements
and [attributes]
. For example, .stars ul
could be replaced with a class like .stars__list
.
src/style.css
Outdated
.stars ul { | ||
list-style: none; | ||
padding: 0; | ||
display: 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.
Since .stars ul
is already using a class for styling, the tag name ul
is not necessary and can be replaced with a class like .stars__list
for consistency and to follow best practices.
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 effort on this Pull Request! 👍 Remember to consider removing unnecessary duplicate property declarations like margin: 0;
in .stars__list
if it's already defined in .stars
. Consistency with margins is key, so stick to top or bottom margins to avoid margin collapse. Avoid styling with tag names, and keep using classes instead. Also, don't forget to add empty lines between sibling blocks for better readability, and maintain consistent indentation throughout your code. Keep up the good work, and use these tips to enhance your code in future projects! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<ul class="stars__list"> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> |
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 and add some 'air' to the code structure. There should be an empty line before and after each div
block containing the stars__list
.
<ul class="stars__list"> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> |
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.
Ensure that empty lines are added between sibling blocks for readability, as mentioned previously.
<ul class="stars__list"> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> |
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 add empty lines between sibling blocks for improved readability.
<ul class="stars__list"> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> |
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 an empty line before and after this block to maintain consistency and readability.
<ul class="stars__list"> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> |
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 add empty lines between sibling blocks to keep the code clean and easy to read.
<ul class="stars__list"> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
<li class="stars__star"></li> | ||
</ul> |
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.
As with the other blocks, add empty lines around this block for better code structure and readability.
.stars { | ||
margin: 0; | ||
} | ||
|
||
.stars__list { | ||
list-style: none; | ||
margin: 0; | ||
padding: 0; |
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
). Style all elements using .classes
. Here, you're styling the .stars
and .stars__list
classes, which is good, but ensure that any other tag names are not styled directly in the rest of your CSS.
@@ -2,0 +1,144 @@ | |||
.stars { | |||
margin: 0; |
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.
Be consistent with your margins. Add only top or bottom margins, but not both, to avoid potential margin collapse. In this case, you have set margin: 0;
which is acceptable for zeroing out all margins, but be mindful of this rule when setting individual margins.
.stars__list { | ||
list-style: none; | ||
margin: 0; | ||
padding: 0; | ||
display: 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.
Consider removing the margin: 0;
declaration from .stars__list
if it's not necessary, as the .stars
class already zeroes out the margin. This would adhere to the best practice of not declaring the same property more than necessary.
its finished code for reviev