Skip to content
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

Develop #5370

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Develop #5370

wants to merge 5 commits into from

Conversation

230220
Copy link

@230220 230220 commented Feb 4, 2025

  • Yellow stars are added with container modifier + pseudo-selector (NO extra classes)
  • Each BEM block has its own separate file
  • All Typical Mistakes from BEM lesson theory are checked.
  • Code follows all the Code Style Rules ❗️

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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 task! 😊 However, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. Active Stars Implementation: The current implementation does not properly differentiate between active and inactive stars as required. Ensure that the stars--N modifier correctly activates the first N stars.

  2. CSS for Active Stars: The task specifies using a background-image for stars, but there is no CSS to visually differentiate active stars from inactive ones. This is crucial for meeting the task requirements.

  3. Use of Pseudo-selectors: The task requires using pseudo-selectors to style active stars without extra classes. Please update your CSS to utilize pseudo-selectors for this purpose.

  4. BEM Methodology: Each BEM block should have its own separate file. Currently, all styles are in a single CSS file, which does not adhere to the task requirements.

  5. Syntax Error: The .stars--0 class is missing a closing bracket }, which will cause a syntax error. Please fix this to ensure your styles are applied correctly.

Once these issues are resolved, your solution will be much closer to meeting the task requirements. Keep up the great work, and don't hesitate to reach out if you need further assistance! 🌟

✨ 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
Comment on lines 17 to 22
.stars--0 {
background-color: #fff;

background-repeat: no-repeat;
background-position: center;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .stars--0 class is missing a closing bracket }. This will cause a syntax error and prevent the styles from being applied correctly. Make sure to add the closing bracket after the last style declaration.

(cherry picked from commit 5e7df66)
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Great progress! 🎉 Your implementation of the star rating system is well-structured, and you've successfully resolved the syntax error with the .stars--0 class. The CSS now correctly uses pseudo-selectors to differentiate between active and inactive stars, which aligns with the task requirements.

However, there are a few important points to address:

  1. BEM Methodology: Each BEM block should have its own separate CSS file. Currently, all styles are in a single CSS file (style.css). Please ensure that you separate the styles for each BEM block into their own files as per the task requirements.

  2. HTML Structure: Make sure the .stars--0 class is used appropriately in the HTML to reflect the inactive state.

The decision to approve is made to help you move forward with your learning. Please review the comments and make the necessary improvements to your code. Addressing these issues will enhance your understanding and adherence to best practices. Keep up the good work and continue learning from these experiences! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Comment on lines +20 to +22
.stars--0 {
background-color: #fff;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The syntax error with the missing closing bracket for .stars--0 has been resolved. Ensure that this class is used appropriately in the HTML to reflect the inactive state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants