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

add solution #4991

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

add solution #4991

wants to merge 2 commits into from

Conversation

ZorosMG
Copy link

@ZorosMG ZorosMG commented Oct 26, 2024

  • 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 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 project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request:

  1. Use of Extra Classes: The task specifies that yellow stars should be added using a container modifier and pseudo-selector, without extra classes. Currently, the active class is used, which does not align with the requirements.

  2. Universal Selector Usage: The use of the universal selector * for resetting margins is against the guidelines due to performance concerns. It's important to avoid this for better performance.

  3. File Structure: Each BEM block should have its own separate file, but the current implementation does not reflect this structure. This is crucial for maintainability and scalability.

  4. Tag Names for Styling: Avoid using tag names for styling, except for html and body. This includes the use of the * selector, which is not allowed.

  5. Missing Required Files: The images folder and the necessary star images (star.svg and star-active.svg) are not mentioned. These are essential for the background-image property to work correctly.

Please address these issues to ensure the code meets the core requirements and functions as expected. Once these changes are made, we can proceed with the approval. 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
.stars__star {
width: 16px;
height: 16px;
background-image: url(images/star.svg);

Choose a reason for hiding this comment

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

Ensure that the 'images/star.svg' file exists in the specified path. If the image is missing, the stars will not display correctly.

src/style.css Outdated
}

.active {
background-image: url(images/star-active.svg);

Choose a reason for hiding this comment

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

Ensure that the 'images/star-active.svg' file exists in the specified path. If the image is missing, the active stars will not display correctly.

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.

Great job on implementing the star rating system! 🌟 Your code is well on its way, but there are a few areas to improve:

  1. Use of Extra Classes: Avoid using extra classes like active for indicating active stars. Instead, manage the active state with container modifiers and pseudo-selectors.

  2. BEM Structure: Ensure each BEM block has its own CSS file rather than combining all styles into one.

  3. CSS Selector Usage: Refrain from using the universal selector * as it can affect performance. Opt for more specific selectors.

  4. HTML Formatting: Use 2 spaces for indentation in your HTML for consistency.

  5. CSS Class Naming: Follow the BEM naming convention for classes, avoiding generic names like active.

  6. Semantic Tags: Incorporate semantic tags in your HTML to enhance meaning and accessibility.

These points are crucial for refining your code and aligning with best practices. Addressing them will significantly enhance your skills and code quality. 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! ✨

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