-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
finish #5493
base: master
Are you sure you want to change the base?
finish #5493
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.
Your implementation is almost there, but there are a few critical issues that need to be addressed before we can approve it. 🚀
-
Missing Files: The absence of an
images
folder and the necessary image files for the star ratings is a critical issue. These are essential for thebackground-image
functionality to work correctly. -
CSS Requirements: The CSS file lacks styles for the
stars--N
modifiers, which are crucial for ensuring the correct number of active stars. This needs to be managed through CSS using pseudo-selectors. -
HTML Structure: The current use of the
stars__star--active
class does not align with the requirement to use pseudo-selectors for indicating active stars. This needs to be adjusted to meet the task requirements. -
BEM Structure: Each BEM block should be in its own separate file. This is important for maintaining a clean and organized codebase.
Please address these issues to fully meet the task requirements. Once these are resolved, your implementation will be in great shape! Keep up the good work, and don't hesitate to reach out if you need further guidance. 😊
✨ 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.
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 submission! 🎉 While your code has been approved to help you progress in your learning journey, there are several important areas for improvement:
-
CSS Requirements: Your current implementation uses the
stars__star--active
class directly in the HTML, which does not meet the task requirement of using CSS pseudo-selectors to indicate active stars. Consider using pseudo-selectors like:nth-child
to manage the active state of stars based on thestars--N
class. -
BEM Structure: Ensure that each BEM block is in its own separate file as per the task requirements. While you have imported
body.css
andstars.css
intostyle.css
, it's crucial to verify that each BEM block is properly separated. -
HTML Structure: The HTML structure currently uses the
stars__star--active
class, which should be replaced with a pseudo-selector approach for indicating active stars. -
Missing Files: The
images
folder and necessary image files for the star ratings are missing. These are essential for thebackground-image
functionality to work correctly.
Please address these issues in future iterations to enhance the functionality and maintainability of your code. Keep up the good work and continue learning from each experience! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
</div> | ||
|
||
<div class="stars stars--1"> | ||
<div class="stars__star stars__star--active"></div> |
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 use of the stars__star--active
class does not align with the task requirement to use pseudo-selectors for indicating active stars. Consider using CSS pseudo-selectors to manage the active state of stars based on the stars--N
class.
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> |
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.
Similar to the previous comment, the stars__star--active
class should be replaced with a CSS pseudo-selector approach to indicate active stars.
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> |
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.
Again, the stars__star--active
class should be replaced with a CSS pseudo-selector approach to indicate active stars.
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> |
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 stars__star--active
class should be replaced with a CSS pseudo-selector approach to indicate active stars.
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> |
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 stars__star--active
class should be replaced with a CSS pseudo-selector approach to indicate active stars.
.stars__star--active { | ||
background-image: url(images/star-active.svg); | ||
} |
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 use of the stars__star--active
class should be replaced with a CSS pseudo-selector approach to indicate active stars based on the stars--N
class. Consider using pseudo-selectors like :nth-child
to manage the active state of stars.
<div class="stars__star stars__star--active"></div> | ||
<div class="stars__star stars__star--active"></div> |
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.
<div class="stars__star stars__star--active"></div> | |
<div class="stars__star stars__star--active"></div> | |
<div class="stars__star"></div> | |
<div class="stars__star"></div> |
you need to implement it without other classes
DEMO LINK
TEST REPORT LINK
Yellow stars are added with container modifier + pseudo-selector (NO extra classes)
Each BEM block has its own separate file
All
Typical Mistakes
fromBEM
lesson theory are checked.Code follows all the Code Style Rules ❗️