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

Task solution #5167

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

Conversation

MindControl01
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 you've done a great job overall, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. BEM Blocks: The task requires two BEM blocks, card and stars, each in their own file. The card block is not explicitly defined in a separate file as a BEM block, which is a core requirement.

  2. SCSS Variables: SCSS variables should be placed in a dedicated variables file. Currently, they are in the _page.scss file, which does not meet the task's core requirements.

  3. HTML Attribute Placement: The crossorigin attribute should be placed correctly within the <link> tag to ensure proper functionality.

These issues are essential to meet the task's core requirements and ensure the code functions as expected. Please address these points, and feel free to reach out if you have any questions or need further guidance. Keep up the great work! 🌟

✨ 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/index.html Outdated
Comment on lines 8 to 9
<link rel="preconnect" href="https://fonts.gstatic.com"
crossorigin="anonymous">

Choose a reason for hiding this comment

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

The crossorigin attribute should be placed within the <link> tag itself. It should be on the same line as the <link> tag or immediately after it, not on a separate line.

font-weight: 700;
align-items: center;
color: #fff;
border: 1px;

Choose a reason for hiding this comment

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

The border property is incomplete. It should specify the border style and color, not just the width. Consider using border: 1px solid var(--light-blue); to ensure the border is styled correctly.

color: #fff;
border: 1px;
border-radius: 5px;
border-color: var(--light-blue);

Choose a reason for hiding this comment

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

The border-color property is redundant here since the border property should already include the color. If you specify the border completely in line 14, you can remove this line.

@MindControl01
Copy link
Author

DEMO LINK
TEST REPORT 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.

Review Summary

Great progress! 🎉 Your solution has been significantly improved, and you've addressed many of the previous issues:

  1. BEM Blocks: The card and stars BEM blocks are now correctly defined in their own separate files, _card.scss and _stars.scss, respectively.

  2. SCSS Variables: You've successfully moved SCSS variables to a separate file _var.scss, which is a core requirement for maintaining consistency.

  3. HTML Structure: The HTML structure is mostly correct, but remember to add the crossorigin attribute to the <link> tags in the index.html file to ensure proper functionality.

Your dedication to improving the code is evident, and this decision to approve is to help you move forward with your learning journey. 🚀 Please review the comments to further refine your code. Addressing the crossorigin attribute issue is crucial for the functionality, so make sure to fix it in future updates. Keep up the great work and continue learning from these experiences! 😊

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

rel="stylesheet"
href="./styles/index.scss"
/>
<link rel="preconnect" href="https://fonts.googleapis.com">

Choose a reason for hiding this comment

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

The crossorigin attribute is missing in this <link> tag. Ensure to include it to meet the task requirements.

href="./styles/index.scss"
/>
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com">

Choose a reason for hiding this comment

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

The crossorigin attribute is missing in this <link> tag. Ensure to include it to meet the task requirements.

/>
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com">
<link href="https://fonts.googleapis.com/css2?family=Roboto:[email protected]&display=swap" rel="stylesheet">

Choose a reason for hiding this comment

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

The crossorigin attribute is missing in this <link> tag. Ensure to include it to meet the task requirements.

Copy link

@vadiimvooo vadiimvooo left a comment

Choose a reason for hiding this comment

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

Seems like you forgot to deploy your solution. Please feel free to ask for help in fe_chat

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.

3 participants