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

done v1 #5109

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

done v1 #5109

wants to merge 4 commits into from

Conversation

dsliusarchyk
Copy link

No description provided.

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! 😊 However, there are a few critical issues that need to be addressed to meet the core requirements of the task. Specifically, the implementation is missing the stars BEM block, SCSS variables, and the required file structure. Additionally, the background-image path and font usage need to be verified against the task specifications. Please make these changes, and feel free to reach out if you need any help. Keep up the great effort! 🌟

✨ 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 effort on your project! 🎉 While your code is generally well-structured and organized, there are a few critical issues that need attention to fully meet the task requirements:

  1. Missing stars BEM Block: The stars BEM block is not included in your implementation. This block should be rewritten from the Stars task using SCSS and included in both your HTML and SCSS files.

  2. SCSS Variables: While you have used SCSS variables for main values like colors, consider expanding their use to include other repeated values such as font-family, background-color, and border-color for better maintainability.

  3. File Structure: Ensure that your file structure aligns with the task requirements. The stars block should be in its own SCSS file, and the index.css file should be removed if not required.

  4. Background Image Path: Verify and adjust the background-image path to ensure it is relative to index.scss and starts with ../images.

  5. Stylesheet Path: Double-check that the stylesheet path is correct and that the index.css file contains all necessary styles and SCSS variables as per the task description.

Your decision to approve this pull request is to help you move forward with your learning. Please review the comments carefully and address these issues in your future work to avoid similar problems. Keep up the good work and continue improving your code! 🚀

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

src/index.html Outdated
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Product cards</title>
<link href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;700&display=swap" rel="stylesheet" />
<link rel="stylesheet" href="./styles/index.css" />

Choose a reason for hiding this comment

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

Ensure that the stylesheet path is correct and aligns with the task requirements. Verify that the index.css file contains the necessary styles and SCSS variables as per the task description.

src/index.html Outdated
</head>
<body>
<h1>Product cards</h1>
<div class="product-card" data-qa="card">

Choose a reason for hiding this comment

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

The stars BEM block is missing. According to the task requirements, this block should be included in the HTML structure.

@@ -0,0 +1,68 @@
body {
margin: 0;
font-family: Roboto, sans-serif;

Choose a reason for hiding this comment

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

Consider using SCSS variables for the font family and other repeated values to enhance maintainability and consistency across your styles.


.product-card {
width: 200px;
background-color: #fff;

Choose a reason for hiding this comment

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

Consider using SCSS variables for colors like background-color and border-color to make it easier to manage and update these values.

}
.product-card__button {
padding: 8px 16px;
background-color: #007bff;

Choose a reason for hiding this comment

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

Consider using SCSS variables for colors like background-color to make it easier to manage and update these values.

background-color: $hover-color;
}
}
}

Choose a reason for hiding this comment

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

The stars BEM block is missing. According to the task requirements, this block should be included and styled in the SCSS file.

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