stars layout no 1 #4963
stars layout no 1 #4963
Conversation
There was a problem hiding this comment.
Great job on your progress so far! 🚀 However, there are a few issues with your CSS and HTML files that need to be addressed. Please consider the comments and suggestions made to improve your code quality and maintainability. 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,47 @@ | |||
| /> | |||
| </head> | |||
There was a problem hiding this comment.
Your meta tag and link tag are not correctly formatted. If the HTML-element has long attribute values or number of attributes is more than 2 - start each one, including the first, on the new line with 2-space indentation related to tag. Tag’s closing bracket should be on the same level as opening one.
src/index.html
Outdated
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| </div> |
There was a problem hiding this comment.
In HTML, it's better to use semantic tags where possible. Instead of using divs, try to use more descriptive tags like section for your stars block, and article or figure for each star.
src/index.html
Outdated
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| <div class="stars__star"></div> | ||
| </div> |
There was a problem hiding this comment.
The class names should represent the meaning of the content, not the styles or tag names. Instead of 'stars--0', 'stars--1', etc, you can use more semantic names like 'stars--none', 'stars--one', etc. Also, instead of 'stars__star', you can use 'star' or 'star--active' and 'star--inactive' to represent the state of each star.
src/index.html
Outdated
| <div class="stars--5__star"></div> | ||
| <div class="stars--5__star"></div> | ||
| <div class="stars--5__star"></div> | ||
| </div> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML to add some 'air' and simplify reading. But don't add them between parent and child elements.
| font-size: 100%; | ||
| font-family: inherit; | ||
| vertical-align: baseline; | ||
| } |
There was a problem hiding this comment.
You're using the '' selector, which is not recommended as it impacts performance. It's better to set styles only for elements that require them. Zeroing out your margins, paddings or other styles with '' is still inefficient for browser. (Checklist: Don't use '*' selector (it impacts performance))
| font-size: 100%; | ||
| font-family: inherit; | ||
| vertical-align: baseline; | ||
| } |
There was a problem hiding this comment.
You're using tag names for styling, which is not a good practice. It's better to style all elements using '.classes' and if needed with ':pseudo-classes', 'pseudo-elements' and '[attributes]'. (Checklist: Don't use tag names for styling (except 'html' and 'body'))
| font-size: 100%; | ||
| font-family: inherit; | ||
| vertical-align: baseline; | ||
| } |
There was a problem hiding this comment.
You forgot to set a fallback font. Always remember to use fallback fonts - alternative font-family in case the main one doesn't work. (Checklist: Remember to use fallback fonts - alternative font-family in case the main one doesn't work)
| background-repeat: no-repeat; | ||
| margin: 0 2px; | ||
| background-position: left; | ||
| } |
There was a problem hiding this comment.
There is a lot of code duplication in your CSS. You're repeating the same styles for the '.stars--#' and '.stars--#__star' classes. It's better to create a common '.stars' and '.stars__star' class and apply the shared styles there, and then use additional classes to apply unique styles. This will make your code more DRY (Don't Repeat Yourself).
There was a problem hiding this comment.
Hey there! Great job on your work so far. However, it looks like there are some issues with your CSS file that need to be addressed. There's a bit of repetition in your CSS classes that could be simplified. Also, remember to use meaningful class names and correct indentation. Keep up the good work! 💪👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <section class="stars--0"> | ||
| <article class="stars__star"></article> | ||
|
|
||
| <article class="stars__star"></article> | ||
|
|
||
| <article class="stars__star"></article> | ||
|
|
||
| <article class="stars__star"></article> | ||
|
|
||
| <article class="stars__star"></article> |
There was a problem hiding this comment.
Remember about correct indentation between parent and child elements. Each level of nesting, including text, contained inside the element, requires 2-space offset. Also, blank line shouldn't be between parent and child elements.
| @@ -13,6 +13,76 @@ | |||
| /> | |||
| </head> | |||
| <body> | |||
| <h1>Stars</h1> | |||
| <section class="stars--0"> | |||
There was a problem hiding this comment.
Class names should represent the meaning of the content (not the styles or tag names). In your case, 'stars--0' doesn't seem to represent the content. Try to use more meaningful class names.
| </section> | ||
|
|
||
| <section class="stars--1"> | ||
| <article class="stars--1__star"></article> |
There was a problem hiding this comment.
The class name 'stars--1__star' seems to be inconsistent with the class name 'stars__star' used in the same section. Make sure to keep your class names consistent across similar elements.
| </section> | ||
|
|
||
| <section class="stars--2"> | ||
| <article class="stars--2__star"></article> |
There was a problem hiding this comment.
The class name 'stars--2__star' seems to be inconsistent with the class name 'stars__star' used in the same section. Make sure to keep your class names consistent across similar elements.
| </section> | ||
|
|
||
| <section class="stars--3"> | ||
| <article class="stars--3__star"></article> |
There was a problem hiding this comment.
The class name 'stars--3__star' seems to be inconsistent with the class name 'stars__star' used in the same section. Make sure to keep your class names consistent across similar elements.
| </section> | ||
|
|
||
| <section class="stars--5"> | ||
| <article class="stars--5__star"></article> |
There was a problem hiding this comment.
The class name 'stars--5__star' seems to be inconsistent with the class name 'stars__star' used in the same section. Make sure to keep your class names consistent across similar elements.
| font-size: 100%; | ||
| font-family: inherit; | ||
| vertical-align: baseline; | ||
| } |
There was a problem hiding this comment.
The usage of the * selector is not recommended as it impacts performance. It's better to set styles only for elements that require them. Zeroing out your margins, paddings or other styles with '*' is still inefficient for browser.
| .stars--0 { | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
The .stars--0 class seems to be unnecessary as it only sets display: flex; which is repeated for each .stars-- class. Consider creating a single .stars class with display: flex; and apply it to all star elements.
| .stars--1 { | ||
| display: flex; | ||
| } | ||
|
|
||
| .stars--1__star { | ||
| height: 16px; | ||
| width: 16px; | ||
| background-image: url(./images/star-active.svg); | ||
| background-repeat: no-repeat; | ||
| background-position: left; | ||
| margin: 0 2px; | ||
| } |
There was a problem hiding this comment.
The .stars--1 and .stars--1__star classes are almost identical to .stars--2, .stars--2__star, .stars--3, .stars--3__star, .stars--4, .stars--4__star, .stars--5, .stars--5__star. This is a repetition of code. Consider creating a single .stars and .stars__star class and apply it to all star elements.
| .stars__star { | ||
| height: 16px; | ||
| width: 16px; | ||
| background-image: url(./images/star.svg); | ||
| background-repeat: no-repeat; | ||
| margin: 0 2px; | ||
| background-position: left; | ||
| } |
There was a problem hiding this comment.
The .stars__star class is almost identical to the .stars--#__star classes. This is a repetition of code. Consider creating a single .stars__star class and apply it to all star elements. Then, you can modify the background-image property for the active stars only.
There was a problem hiding this comment.
Great effort on your CSS code! 👍 However, there are some areas that need optimization. Avoid using the * selector as it can impact performance. Also, your code has redundancy in the stars classes which can be simplified. Class names should be meaningful and represent the content, not the styles or tag names. Keep learning and improving! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -13,6 +13,76 @@ | |||
| /> | |||
| </head> | |||
| <body> | |||
| <h1>Stars</h1> | |||
| <section class="stars--0"> | |||
There was a problem hiding this comment.
The class name stars--0 does not represent the meaning of the content. Use class names that represent the meaning of the content, not the styles or tag names. Consider renaming this class to something more meaningful.
| @@ -13,6 +13,76 @@ | |||
| /> | |||
| </head> | |||
| <body> | |||
| <h1>Stars</h1> | |||
| <section class="stars--0"> | |||
| <article class="stars__star"></article> | |||
There was a problem hiding this comment.
The class name stars__star does not represent the meaning of the content. Use class names that represent the meaning of the content, not the styles or tag names. Consider renaming this class to something more meaningful.
| <article class="stars__star"></article> | ||
| </section> | ||
|
|
||
| <section class="stars--1"> |
There was a problem hiding this comment.
The class name stars--1 does not represent the meaning of the content. Use class names that represent the meaning of the content, not the styles or tag names. Consider renaming this class to something more meaningful.
| </section> | ||
|
|
||
| <section class="stars--1"> | ||
| <article class="stars--1__star"></article> |
There was a problem hiding this comment.
The class name stars--1__star does not represent the meaning of the content. Use class names that represent the meaning of the content, not the styles or tag names. Consider renaming this class to something more meaningful.
| <article class="stars__star"></article> | ||
| </section> | ||
|
|
||
| <section class="stars--2"> |
There was a problem hiding this comment.
The class name stars--2 does not represent the meaning of the content. Use class names that represent the meaning of the content, not the styles or tag names. Consider renaming this class to something more meaningful.
| <article class="stars__star"></article> | ||
| </section> | ||
|
|
||
| <section class="stars--5"> |
There was a problem hiding this comment.
The class name stars--5 does not represent the meaning of the content. Use class names that represent the meaning of the content, not the styles or tag names. Consider renaming this class to something more meaningful.
| </section> | ||
|
|
||
| <section class="stars--5"> | ||
| <article class="stars--5__star"></article> |
There was a problem hiding this comment.
The class name stars--5__star does not represent the meaning of the content. Use class names that represent the meaning of the content, not the styles or tag names. Consider renaming this class to something more meaningful.
| 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, | ||
| font, | ||
| img, | ||
| ins, | ||
| kbd, | ||
| q, | ||
| s, | ||
| samp, | ||
| small, | ||
| strike, | ||
| strong, | ||
| sub, | ||
| sup, | ||
| tt, | ||
| var, | ||
| dl, | ||
| dt, | ||
| dd, | ||
| ol, | ||
| ul, | ||
| li, | ||
| fieldset, | ||
| form, | ||
| label, | ||
| legend, | ||
| table, | ||
| caption, | ||
| tbody, | ||
| tfoot, | ||
| thead, | ||
| tr, | ||
| th, | ||
| td { | ||
| margin: 0; | ||
| padding: 0; | ||
| border: 0; | ||
| outline: 0; | ||
| font-weight: inherit; | ||
| font-style: inherit; | ||
| font-size: 100%; | ||
| font-family: inherit; | ||
| vertical-align: baseline; | ||
| } |
There was a problem hiding this comment.
You are using the * selector to reset CSS. This can impact performance as it selects all elements. Try to be more specific with your selectors or use a CSS reset library.
| .stars--0 { | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
The classes .stars--0, .stars--1, .stars--2, .stars--3, .stars--4, .stars--5 all have the same display: flex; property. You can simplify your code by creating a single .stars class with this property and applying it to all your stars elements.
| .stars--1__star { | ||
| height: 16px; | ||
| width: 16px; | ||
| background-image: url(./images/star-active.svg); | ||
| background-repeat: no-repeat; | ||
| background-position: left; | ||
| margin: 0 2px; | ||
| } |
There was a problem hiding this comment.
The classes .stars--1__star, .stars--2__star, .stars--3__star, .stars--4__star, .stars--5__star all have the same properties. You can simplify your code by creating a single .stars__star class with these properties and applying it to all your star elements.
DEMO LINK
TEST REPORT LINK
[x ] Yellow stars are added with container modifier + pseudo-selector (NO extra classes)
[ x] Each BEM block has its own separate file
[x ] All
Typical MistakesfromBEMlesson theory are checked.[x ] Code follows all the Code Style Rules ❗️