solution #4961
solution #4961
Conversation
There was a problem hiding this comment.
Great job so far! 😄 However, there are a few areas that need improvement. Please use semantic tags instead of div for the stars blocks, and ensure the path to the images is consistent. Also, remember to add empty lines between multiline sibling blocks of HTML for readability. Don't forget to include a description for the content of your webpage using the <meta name="description" content="..."> tag in your <head> section. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -12,7 +13,49 @@ | |||
| href="./style.css" | |||
| /> | |||
There was a problem hiding this comment.
HTML Formatting: The attributes of your link tag are not correctly formatted. Each attribute should start on a new line with a 2-space indentation relative to the tag.
src/index.html
Outdated
| <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> |
There was a problem hiding this comment.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div with the class stars.
src/index.html
Outdated
| <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> |
There was a problem hiding this comment.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div with the class stars.
src/index.html
Outdated
| <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> |
There was a problem hiding this comment.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div with the class stars.
src/index.html
Outdated
| <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> |
There was a problem hiding this comment.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div with the class stars.
src/index.html
Outdated
| <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> | ||
| </div> |
There was a problem hiding this comment.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div with the class stars.
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| </div> |
There was a problem hiding this comment.
HTML Content: It would be better to use semantic tags instead of div for the stars blocks. Consider using a semantic tag like section or article that would better describe the content.
src/style.css
Outdated
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
Avoid using the * selector as it impacts performance. It's better to set styles only for elements that require them. You can find this in the task checklist under the CSS section.
| .stars--1 :first-child { | ||
| background-image: url(./images/star-active.svg); | ||
| } |
There was a problem hiding this comment.
The path to the images is not consistent. Here you're using ./images/star-active.svg, while in other places you're using images/star-active.svg. It's not an error, but it's better to keep the code consistent.
| .stars--5 :nth-child(-n + 1), | ||
| .stars--5 :nth-child(-n + 2), | ||
| .stars--5 :nth-child(-n + 3), | ||
| .stars--5 :nth-child(-n + 4), | ||
| .stars--5 :nth-child(-n + 5) { | ||
| background-image: url(images/star-active.svg); | ||
| } |
There was a problem hiding this comment.
You can simplify the CSS rule for .stars--5 by using :nth-child(-n + 5) only. There's no need to repeat it for each child.
There was a problem hiding this comment.
Great job on your code! 👍 However, there are a few areas that need some improvement. Please consider using a fallback font, avoid fixing container size, be consistent with your margins, and avoid using the * selector for styling. Also, ensure the indentation for the attributes is consistent across the document and consider using a templating system or a loop to avoid code repetition. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| <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> |
There was a problem hiding this comment.
This block of code is repeated multiple times with only minor changes. Consider using a templating system or a loop in a scripting language to generate these elements programmatically. This will make your code more maintainable and less prone to errors.
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
Avoid using the * selector for styling. It impacts performance as it affects all elements. Try to set styles only for elements that require them.
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
There's a checklist item that suggests not to use tag names for styling, except for html and body.
| border-radius: 0.5px; | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
| margin-right: 4px; |
There was a problem hiding this comment.
Remember to use fallback fonts. It's an alternative font-family in case the main one doesn't work. You can specify a list of fonts separated by commas. The browser will use the first font in the list available on the user's computer.
| background-repeat: no-repeat; | ||
| background-position: center; | ||
| margin-right: 4px; | ||
| } |
There was a problem hiding this comment.
Be consistent with your margins. It seems you are applying the margin-right property to all .stars__star elements. This could potentially lead to margin collapse. Consider applying margins only to the top or bottom, but not both.
| .stars--5 :nth-child(-n + 4), | ||
| .stars--5 :nth-child(-n + 5) { | ||
| background-image: url(images/star-active.svg); | ||
| } |
There was a problem hiding this comment.
It's a good practice to avoid fixing container size if there is no such a requirement. Let the content size dictate it. This can help to avoid overflow or accidental scroll bar.
There was a problem hiding this comment.
Great work on the HTML and CSS! Your code is well-structured and readable. There are a few minor issues that need to be addressed, including the use of the * selector and the placement of empty lines in your HTML. These aren't major issues, but they are worth noting for future projects. Keep up the good work and continue to refine your coding skills! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <div class="stars__star"></div> | ||
|
|
||
| <div class="stars__star"></div> | ||
| </div> |
There was a problem hiding this comment.
According to the checklist, you should add empty lines between multiline sibling blocks of HTML to simplify reading. However, you have added empty lines between child elements, which is not recommended. You should remove the empty lines between the div elements with the class stars__star.
| body { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
According to the checklist, you should avoid using the * selector for styling, as it impacts performance. Although this is a minor issue, it's a good practice to style only the elements that require it.
| .stars--5 :nth-child(-n + 1), | ||
| .stars--5 :nth-child(-n + 2), | ||
| .stars--5 :nth-child(-n + 3), | ||
| .stars--5 :nth-child(-n + 4), | ||
| .stars--5 :nth-child(-n + 5) { |
There was a problem hiding this comment.
There seems to be a mistake in your CSS selectors. The :nth-child(-n + 1) selector in the .stars--5 rule will only select the first child, but you probably intended to select all children. To select all children, you can simply use the :nth-child(n) selector. Also, there's no need to repeat the same selector for each child. You can combine them into a single rule: .stars--5 :nth-child(n).
Typical MistakesfromBEMlesson theory are checked.