add task solution #4882
add task solution #4882
Conversation
OlhaLishenko
commented
Oct 2, 2024
OlhaLishenko
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
While you're doing great, there are a few important areas that need adjustment before we can move forward. 🛠 Remember to keep an eye on the BEM convention for class names, ensure proper formatting of attributes in your HTML, and maintain a consistent approach to margins and container sizes in your CSS. These changes will greatly enhance the maintainability and scalability of your code. Keep up the hard work, and you'll have this polished in no time! 💪
✨ 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 stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| </div> | ||
| <div class="stars stars--1"> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| </div> | ||
| <div class="stars stars--2"> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| </div> | ||
| <div class="stars stars--3"> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| </div> | ||
| <div class="stars stars--4"> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| </div> | ||
| <div class="stars stars--5"> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> |
There was a problem hiding this comment.
The class names stars stars--0, stars stars__star, etc., seem to be a mixture of BEM naming convention and something else. According to BEM, modifiers (--modifier) and elements (__element) should not be used together with the block name. Consider revising the class names to follow BEM properly. For example, the block could be stars, the element could be stars__star, and the modifier could be stars--0. Ensure that the class names represent the meaning of the content and not the styles or tag names.
src/index.html
Outdated
| <div class="stars stars--0"> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> | ||
| <div class="stars stars__star"></div> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML to improve readability. There should be an empty line between each div.stars block.
src/style.css
Outdated
| body { | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
Consider using a class for styling instead of the body tag directly, unless there is a specific reason to do so. Tag selectors can cause specificity issues and are not as reusable as class selectors.
src/style.css
Outdated
| .stars__star { | ||
| background-image: url(images/star.svg); | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
| width: 16px; | ||
| height: 16px; | ||
| margin-right: 4px; | ||
| } |
There was a problem hiding this comment.
To ensure your CSS is robust and adaptable, avoid fixing the size of containers like .stars__star. If there is no explicit requirement to fix the size, consider using padding and allow the size to be determined by the content or context.
src/style.css
Outdated
| background-position: center; | ||
| width: 16px; | ||
| height: 16px; | ||
| margin-right: 4px; |
There was a problem hiding this comment.
Be consistent with your margins. It's a good practice to add margin on the top or bottom only. This approach helps to prevent issues with margin collapsing and makes the margin behavior more predictable.
There was a problem hiding this comment.
Great job on your CSS code! 👍 It's well-structured and follows the guidelines closely. Just remember for next time to consider adding margins to one side and including a fallback font-family for better cross-browser compatibility. Also, pay attention to the formatting in your HTML for readability. Keep up the good work and happy coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <html | ||
| lang="en" | ||
| class="page" | ||
| > |
There was a problem hiding this comment.
The opening <html> tag and its attributes should be formatted properly with each attribute starting on a new line with a 2-space indentation relative to the tag.
| <body class="page__body"> | ||
| <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.
Remember to add empty lines between multiline sibling blocks of HTML to add some 'air' and simplify reading. There should be an empty line between the </div> and the next <div class="stars stars--1">, and so on for each group of stars.
| .page__body { | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
It's a good practice to provide a fallback font-family in case the main one doesn't load. Consider adding a generic font family like 'sans-serif' as a fallback.
| background-position: center; | ||
| width: 16px; | ||
| height: 16px; | ||
| margin: 0 4px 0 0; |
There was a problem hiding this comment.
To maintain consistency in your margins and to avoid potential margin collapse issues, it's recommended to add margins to only one side (either top or bottom, but not both). Here you have a right margin, which is fine, but just be mindful of vertical margins.
