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

product card #5277

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

Conversation

katarzynamilewska
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 mostly on point, there are a few critical issues that need to be addressed before we can proceed. Specifically, SCSS variables should be used and placed in a separate file, and the BEM blocks card and stars need to be separated into individual SCSS files. These changes are essential to meet the core requirements of the task. Once these are resolved, you'll be all set! Keep up the great work, and don't hesitate to reach out if you need any help. 🚀

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

Review Summary

Great job on your submission! 🎉 Your code meets the task requirements and constraints effectively. Here’s a quick summary of what you did well and areas for improvement:

  • SCSS Variables: You've correctly used SCSS variables and placed them in a separate file named _variables.scss. This promotes reusability and consistency across your styles.

  • BEM Blocks: The separation of BEM blocks into individual SCSS files (card.scss and stars.scss) is well-executed. This modular approach enhances maintainability.

  • SCSS Nesting: You've utilized SCSS nesting for elements, modifiers, and pseudo-classes in both card.scss and stars.scss, which is great for readability and organization.

  • Attributes and Styles: The required data-qa attributes are present in the HTML file, ensuring that your code meets the task specifications.

  • Image Paths: The background-image paths in stars.scss are correctly set relative to index.scss, which is crucial for proper asset loading.

  • Font Usage: The required font is imported from Google Fonts, aligning with the task requirements.

While your code is approved, please pay attention to the following critical issue that should be addressed in future projects:

  • SCSS File Import: Ensure that the SCSS files are correctly structured and linked, as there was a critical issue with the SCSS file import initially. Proper linking is essential for the functionality of your styles.

This decision is made to help you move forward with your learning. Please review the comments and continue to improve your code. Keep up the great work and learn from any mistakes to enhance your skills further! 🚀

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

<link
href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap"
rel="stylesheet"
/>
<link
rel="stylesheet"
href="./styles/index.scss"

Choose a reason for hiding this comment

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

The SCSS file index.scss is linked here. Ensure that SCSS variables are used and placed in a separate file, and that the BEM blocks card and stars are separated into individual SCSS files as per 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