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 #5286

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

Develop #5286

wants to merge 3 commits into from

Conversation

Cr1staliss
Copy link

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! 😊 While your code is well-structured and mostly aligns with best practices, there are a few critical issues that need to be addressed to meet the core requirements of the task:

  1. BEM Blocks: The card and stars blocks need to be separated into individual SCSS files as per the task requirements.
  2. SCSS Variables: These should be placed in a separate file for better organization and maintainability.
  3. Background Image URL: Ensure the URL in the .stars_star class is relative to index.scss and starts with ../images.
  4. Stars Block Modifiers: Implement modifiers like stars--N to highlight the first N stars.
  5. Unnecessary Files: Remove the index.css file as it is not needed for the task.

Please address these issues to fully meet the task requirements. Keep up the great work, and I'm confident you'll nail it! 🚀

✨ 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.

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 making progress with your project! 🎉 While there are a few areas that need attention, your efforts are commendable. Here are some key points to focus on:

  1. BEM Blocks: Ensure that the card and stars blocks are separated into individual SCSS files for better organization.

  2. SCSS Variables: Consider moving SCSS variables to a separate file to enhance maintainability and organization.

  3. Stars Block Modifiers: Implement modifiers like stars--N to highlight the first N stars, as this is crucial for meeting the task requirements.

Your decision to move forward with the Pull Request is to encourage your learning journey. Please review the comments and address these issues to improve your code. Remember, addressing these points will not only help you meet the task requirements but also enhance your coding skills. 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 +1 to +3
$main-accent: #060b35;
$secondary: #616070;
$blue-accent: #00acdc;

Choose a reason for hiding this comment

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

Consider moving these SCSS variables to a separate file for better organization and maintainability, as per the task requirements.

Comment on lines +61 to +82
.stars {
display: flex;
flex-direction: row;

&_star {
background-image: url(../images/star-active.svg);
background-position: center;
background-repeat: no-repeat;
display: block;
margin-right: 4px;

width: 16px;
height: 16px;
}

& > :nth-child(5) {
background-position: center;
background-repeat: no-repeat;

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.

The task requires implementing modifiers like stars--N to highlight the first N stars. Consider adding these modifiers to meet the task requirements.

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