Stars block #5296
Stars block #5296
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. The use of <li> tags without a parent <ul> or <ol> is semantically incorrect and needs to be corrected to ensure proper HTML structure. Additionally, the CSS issue with the nth-child(-n + 0) selector should be fixed to ensure the intended styling is applied. Once these issues are resolved, your code will be in great shape! Keep up the great work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
src/index.html
Outdated
| <div class="stars stars--0"> | ||
| <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> |
There was a problem hiding this comment.
The <li> elements should be contained within a <ul> or <ol> element to be semantically correct. Consider wrapping these <li> elements with a <ul> tag.
src/index.html
Outdated
| <div class="stars stars--1"> | ||
| <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> |
There was a problem hiding this comment.
The <li> elements should be contained within a <ul> or <ol> element to be semantically correct. Consider wrapping these <li> elements with a <ul> tag.
src/index.html
Outdated
| <div class="stars stars--2"> | ||
| <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> |
There was a problem hiding this comment.
The <li> elements should be contained within a <ul> or <ol> element to be semantically correct. Consider wrapping these <li> elements with a <ul> tag.
src/index.html
Outdated
| <div class="stars stars--3"> | ||
| <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> |
There was a problem hiding this comment.
The <li> elements should be contained within a <ul> or <ol> element to be semantically correct. Consider wrapping these <li> elements with a <ul> tag.
src/index.html
Outdated
| <div class="stars stars--4"> | ||
| <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> |
There was a problem hiding this comment.
The <li> elements should be contained within a <ul> or <ol> element to be semantically correct. Consider wrapping these <li> elements with a <ul> tag.
src/index.html
Outdated
| <div class="stars stars--5"> | ||
| <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> |
There was a problem hiding this comment.
The <li> elements should be contained within a <ul> or <ol> element to be semantically correct. Consider wrapping these <li> elements with a <ul> tag.
src/style.css
Outdated
| .stars--0 .stars__star:nth-child(-n + 0) { | ||
| background-image: url(./images/star.svg); | ||
| } |
There was a problem hiding this comment.
The nth-child(-n + 0) selector does not select any elements because the formula results in zero or negative numbers, which are not valid child indices. Consider adjusting this selector to correctly apply styles to the intended elements.
There was a problem hiding this comment.
Review Summary
Great progress! 🎉 Your code has been reviewed and is approved. Here are some key points to consider for future improvements:
-
HTML Structure: Ensure that the
<html>tag is not self-closing. It should be written as<html lang="en" class="page">without the closing slash. This is crucial for maintaining valid HTML syntax. -
CSS Selectors: The
nth-childselector issue has been resolved, but remember that selectors likenth-child(-n + 0)are invalid and should be avoided. Your use ofnth-child(-n + 1)is correct, so keep up the good work! -
Semantic HTML: While using
<div>tags for stars is acceptable, always aim for a semantic structure in your HTML. This will improve the accessibility and maintainability of your code. -
File Structure: Ensure all necessary files are included and that there are no unrelated files in your project. This helps in keeping your project organized and easy to navigate.
You've done a great job addressing previous issues, and this approval is to help you move forward with your learning. Please review the comments to further improve your code. Remember, addressing critical issues is essential for the functionality of your code and to avoid similar problems in the future. Keep learning and improving! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -1,6 +1,9 @@ | |||
| <!doctype html> | |||
| <html lang="en"> | |||
| <head> | |||
| <html | |||
There was a problem hiding this comment.
The <html> tag should not be self-closing. It should be written as <html lang="en" class="page"> without the closing slash.
| background-image: url(./images/star.svg); | ||
| } | ||
|
|
||
| .stars--1 .stars__star:nth-child(-n + 1) { |
There was a problem hiding this comment.
The selector nth-child(-n + 1) is correct here, but ensure that any similar selectors like nth-child(-n + 0) are corrected, as they are not valid and will not select any elements.
Typical MistakesfromBEMlesson theory are checked.