add task solution #4936
add task solution #4936
Conversation
There was a problem hiding this comment.
You've done a great job so far! 👍 However, there are some issues with your CSS file that need to be addressed. Try to avoid styling elements directly and using the * selector due to its impact on performance. Also, remember to format your HTML attributes correctly and add empty lines between sibling elements for better readability. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <div class="container"> | ||
| <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> | ||
| </div> |
There was a problem hiding this comment.
You should add empty lines between sibling elements to improve readability. This is especially important for multi-line blocks. But remember, don't add them between parent and child elements.
src/style.css
Outdated
| font-size: 100%; | ||
| font-size: inherit; | ||
| vertical-align: baseline; | ||
| } |
There was a problem hiding this comment.
You're using the * selector which is not recommended due to its impact on performance. It's better to set styles only for elements that require them.
src/style.css
Outdated
| html, | ||
| body, | ||
| div, | ||
| span, | ||
| applet, | ||
| object, | ||
| iframe, | ||
| h1, | ||
| h2, | ||
| h3, | ||
| h4, | ||
| h5, | ||
| h6, | ||
| p, | ||
| blockquote, | ||
| pre, | ||
| a, | ||
| abbr, | ||
| acronym, | ||
| address, | ||
| big, | ||
| cite, | ||
| code, | ||
| del, | ||
| dfn, | ||
| em, | ||
| img, | ||
| ins, | ||
| kbd, | ||
| q, | ||
| s, | ||
| samp, | ||
| small, | ||
| strike, | ||
| strong, | ||
| sub, | ||
| sup, | ||
| tt, | ||
| var, | ||
| b, | ||
| u, | ||
| i, | ||
| center, | ||
| dl, | ||
| dt, | ||
| dd, | ||
| ol, | ||
| ul, | ||
| li, | ||
| fieldset, | ||
| form, | ||
| label, | ||
| legend, | ||
| table, | ||
| caption, | ||
| tbody, | ||
| tfoot, | ||
| thead, | ||
| tr, | ||
| th, | ||
| td, | ||
| article, | ||
| aside, | ||
| canvas, | ||
| details, | ||
| embed, | ||
| footer, | ||
| header, | ||
| hgroup, | ||
| menu, | ||
| nav, | ||
| output, | ||
| ruby, | ||
| section, | ||
| summary, | ||
| time, | ||
| mark, | ||
| audio, | ||
| video { |
There was a problem hiding this comment.
You're styling a lot of elements using their tag names, which goes against the checklist. The only exceptions should be the html and body tags. It's recommended to style elements using .classes, and if needed with :pseudo-classes, pseudo-elements and [attributes].
src/style.css
Outdated
| ol, | ||
| ul { | ||
| list-style: none; | ||
| } |
There was a problem hiding this comment.
Here you're styling the ol and ul tags directly, which is not recommended. You should use classes to style these elements.
| display: flex; | ||
| } | ||
|
|
||
| .stars__star { | ||
| box-sizing: border-box; | ||
| background-repeat: no-repeat; | ||
| height: 16px; | ||
| width: 16px; | ||
| background-position: center; | ||
| background-image: url('./images/star.svg'); | ||
| } |
There was a problem hiding this comment.
This part of the code is well written. You've used classes for styling, which is good practice.
| .stars__star:not(:first-child) { | ||
| margin-left: 4px; | ||
| } |
There was a problem hiding this comment.
This is a good use of the :not(:first-child) pseudo-class. It helps to avoid unnecessary left margin on the first star.
| .stars--1 .stars__star:nth-child(-n + 1), | ||
| .stars--2 .stars__star:nth-child(-n + 2), | ||
| .stars--3 .stars__star:nth-child(-n + 3), | ||
| .stars--4 .stars__star:nth-child(-n + 4), | ||
| .stars--5 .stars__star:nth-child(-n + 5) { | ||
| background-image: url('./images/star-active.svg'); | ||
| } |
There was a problem hiding this comment.
Good use of the nth-child pseudo-class and the :not pseudo-class to control the display of active stars.
There was a problem hiding this comment.
Hey there! Your work on the HTML structure is great! 👍 However, there are a few issues with your CSS file that need to be addressed. Firstly, try to avoid using tag names for styling. Using .classes, :pseudo-classes, pseudo-elements and [attributes] is a better practice. Secondly, the use of * selector can impact the performance of your site. It's better to set styles only for elements that require them. Keep up the good work and make these small changes to improve your code! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/style.css
Outdated
| font-size: 100%; | ||
| font-size: inherit; | ||
| vertical-align: baseline; | ||
| } |
There was a problem hiding this comment.
You are using * selector which impacts the performance. Set styles only for elements that require them. Zeroing out your margins, paddings or other styles with '*' is still inefficient for browser.
src/style.css
Outdated
| font-size: 100%; | ||
| font-size: inherit; | ||
| vertical-align: baseline; | ||
| } |
There was a problem hiding this comment.
You are using tag names for styling. It's recommended to style all elements using .classes and if needed with :pseudo-classes, pseudo-elements and [attributes].
There was a problem hiding this comment.
Great job on your project! There are a few minor issues that need to be addressed in your HTML and CSS files. Please review the comments and make the necessary adjustments. This decision was made to help you move forward with your learning. Keep up the good work and continue to improve your code. Remember, practice makes perfect! 🚀👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <div class="container"> | ||
| <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> | ||
| </div> |
There was a problem hiding this comment.
According to the checklist, you should add an empty line between multiline sibling blocks of HTML. This can make your code more readable.
| body{ | ||
| margin: 0; | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
According to the checklist, you should avoid using tag names for styling (except html and body). In this case, you are using the body tag which is acceptable. However, you are using the * selector to set styles for all elements which can impact performance. It's better to set styles only for elements that require them.
| width: 16px; | ||
| background-position: center; | ||
| background-image: url('./images/star.svg'); | ||
| } |
There was a problem hiding this comment.
It's a good practice to provide alternative images in case the main one doesn't work or takes time to load. You can use the CSS background property to set multiple background images. If the first background image is not found, the next one will be used.
|
|
||
| .stars__star:not(:first-child) { | ||
| margin-left: 4px; | ||
| } |
There was a problem hiding this comment.
Be consistent with your margins. It's better to add only top or bottom margins, but not both, to avoid potential margin collapse. Here, you are adding a left margin which is fine as long as you are consistent with it throughout your CSS.
Typical MistakesfromBEMlesson theory are checked.