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 task solution #5001

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

Conversation

Dariya-Goncharovskaya
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! 😊 There are a few issues that need to be addressed before we can proceed. The data-qa attribute is incorrectly applied to multiple cards and is missing on the "Buy" link inside the first card. Additionally, there's an unnecessary checklist file that should be removed. Please make these adjustments to ensure your code fully meets the task requirements. 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 job on addressing the issues with the data-qa attributes and ensuring the SCSS files are well-structured! 🎉 You've made significant progress, and your attention to detail is commendable. However, there are a few points to keep in mind for future improvements:

  • data-qa Attributes: You've correctly applied the data-qa attributes to the first card and the "Buy" link, but ensure each card has a unique data-qa attribute if required by the task.

  • SCSS Improvements: The place-content property is not suitable for block elements. Consider using display: flex with justify-content: center and align-items: center for centering.

  • Variable Definition: Ensure that the variable $product-code-color is defined in the /src/styles/utils/variables file to prevent any issues with undefined variables.

  • Unnecessary Files: Remember to remove any unnecessary files like the checklist file to keep your project clean.

Keep up the excellent work, and continue refining your skills! Your dedication to improving your code is evident, and addressing these points will help you in future projects. 🚀

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

Comment on lines +143 to +144
<div
class="card"

Choose a reason for hiding this comment

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

The data-qa attribute is missing on this card. Ensure that each card has a unique data-qa attribute if required by the task.

Comment on lines +171 to +172
<a
class="card__buy"

Choose a reason for hiding this comment

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

The data-qa attribute is missing on the 'Buy' link inside this card. Ensure that the 'Buy' link inside the first card has the data-qa attribute as required.

Comment on lines +102 to +103
display: block;
place-content: center center;

Choose a reason for hiding this comment

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

The place-content property is not valid for block elements. Consider using display: flex with justify-content: center and align-items: center to achieve centering.

font-family: Roboto, sans-serif;
font-size: 10px;
line-height: 14px;
color: $product-code-color;

Choose a reason for hiding this comment

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

Ensure that the variable $product-code-color is defined in the /src/styles/utils/variables file to prevent any issues with undefined variables.

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