-
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 #5012
base: master
Are you sure you want to change the base?
add task solution #5012
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 implementation is mostly on point, there are a few critical issues that need to be addressed before we can proceed:
-
CSS Styling: The use of tag names for styling in your CSS file goes against the requirement to use class selectors instead. This change is important to ensure that your styles are modular and do not unintentionally affect other elements.
-
HTML Formatting: The HTML file should consistently use 2 spaces for indentation. Currently, it uses a mix of spaces and tabs, which can lead to inconsistencies and make the code harder to read.
-
Missing Assets: The
images
folder and thestar.svg
andstar-active.svg
images are missing. These are necessary for thebackground-image
property in your CSS to work correctly.
Please address these issues, and feel free to reach out if you need any help. Once these 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/style.css
Outdated
|
||
display: flex; | ||
justify-content: space-between; | ||
flex-direction: column; |
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 flex-direction: column;
on the .container
class might not be necessary if you intend to display the stars horizontally. Consider using flex-direction: row;
or removing this line if the default row direction is desired.
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 constraints, and I'm approving it to help you move forward with your learning journey. However, there are a few areas for improvement:
-
CSS Margin Issue: The use of
margin
for the.stars__star
elements is inconsistent with the requirement to avoid vertical margins between blocks. Please review this and adjust accordingly. -
HTML Formatting: Ensure that the indentation and formatting follow the 2-space rule as specified in the checklist. This will help maintain consistency and readability.
-
General Requirements: You've done well to reset the browser's default
margin
with the.page
class settingmargin: 0
, and the use ofbackground-image
for stars is correctly implemented.
Please address these issues to enhance your code quality. Remember, learning from these observations will help you avoid similar problems in the future. Keep up the great work and continue improving your skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
Typical Mistakes
fromBEM
lesson theory are checked.