-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add task solution #5017
base: master
Are you sure you want to change the base?
add task solution #5017
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on this project! 😊 While your code is mostly on the right track, there are a few critical issues that need to be addressed before we can proceed:
-
CSS Selector Logic: The
:nth-child
selectors in your CSS are not correctly configured, which affects the functionality of the star rating system. For example,.stars--0 .stars__star:nth-child(-n+0)
will not select any elements, and.stars--1 .stars__star:nth-child(-n+1)
only selects the first child. This needs to be corrected to ensure the intended number of active stars is displayed. -
BEM Methodology: The task requires using BEM methodology with separate CSS files for each BEM block. Currently, all styles are in a single CSS file, which does not meet the core requirements.
These issues are critical as they impact the functionality and structure of your code. Once these are resolved, your solution will be much stronger. Keep up the great work, and I'm confident you'll nail it with these adjustments! 🚀
✨ 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.
.stars--0 .stars__star:nth-child(-n+0), | ||
.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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The :nth-child
selectors are incorrectly configured. For example, .stars--0 .stars__star:nth-child(-n+0)
will not select any elements because -n+0
results in no valid index. Similarly, .stars--1 .stars__star:nth-child(-n+1)
will only select the first child. Consider adjusting these selectors to correctly reflect the intended number of active stars. For instance, .stars--1 .stars__star:nth-child(-n+1)
should be .stars--1 .stars__star:nth-child(-n+2)
to select the first two stars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! 😊 It looks like there are a few critical issues that need to be addressed before we can approve the pull request. Here’s a quick rundown:
-
Universal Selector Usage: The use of the universal selector
*
in your CSS for resetting margins can impact performance. It's recommended to avoid this and use more specific selectors. -
BEM Structure: Each BEM block should have its own separate file, but the current setup doesn't reflect this requirement. This is crucial for maintaining a scalable and organized codebase.
-
HTML Formatting: The HTML file has inconsistent indentation. Following a consistent 2-space indentation is important for readability and maintaining coding standards.
-
CSS Class Naming: Class names should reflect the meaning of the content rather than the style or tag names. For example,
stars__star
could be more descriptive of the content it represents. -
Semantic HTML: Using semantic tags enhances SEO and accessibility. Consider revising your HTML to incorporate these where possible.
-
nth-child
Selector Issue: The pattern-n+0
in your CSS will not select any elements, which might not be what you intended. Adjust this to ensure it selects the correct elements.
Addressing these points will help ensure your code meets the task requirements and is robust for future development. Keep up the great work, and I’m sure you’ll have this sorted in no time! 🚀
✨ 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.
.stars--0 .stars__star:nth-child(-n+0), | ||
.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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nth-child
selectors are using a pattern -n+X
where X is the number of stars to be styled. However, the pattern -n+0
in line 17 will not select any elements, which might not be the intended behavior. You might want to adjust this to ensure it selects the correct number of stars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your submission! 🎉 Your code meets most of the task requirements, and I'm approving it to help you move forward with your learning journey. However, there are a few areas for improvement:
- HTML Structure: Avoid redundant classes like
stars
on innerdiv
elements within thesection
with the same class. - CSS Styling: Refrain from using tag names for styling, except for
html
andbody
. Also, consider adding fallback fonts for better compatibility. - File Structure: Ensure each BEM block has its own file as per the task requirements.
- Semantic Tags: Double-check that all elements use the most semantically appropriate tags.
- Logical Error in CSS: Adjust the
:nth-child
selectors to correctly select the intended number of active stars.
Please review these comments to improve your code. Addressing these issues will enhance your skills and help you avoid similar problems in the future. Keep up the great work and continue learning from your experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
.stars--0 .stars__star:nth-child(-n+0), | ||
.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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a logical error in the ':nth-child' selectors. The '-n+0' selector in '.stars--0 .stars__star:nth-child(-n+0)' will not select any elements, as it effectively means 'select none'. Similarly, the other selectors should be adjusted to correctly select the intended number of active stars. For example, '.stars--1 .stars__star:nth-child(-n+1)' should be '.stars--1 .stars__star:nth-child(-n+2)' to select the first star, and so on.
No description provided.