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

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

Conversation

Maks-Chervyakov
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! 😊 There are a few critical issues that need to be addressed before we can approve the pull request:

  1. SCSS Compilation: The HTML file is linking directly to an SCSS file. Browsers cannot interpret SCSS, so you need to compile your SCSS into CSS and link the resulting CSS file in your HTML.

  2. Variable Definitions: Ensure that all SCSS variables ($white, $gray-elements, $main-accent, $secondary, $blue-accent) are defined in your variables.scss file or imported correctly. This is crucial for maintaining the styling consistency across your project.

  3. Image Paths: Verify that the star-active.svg and star.svg images exist in the ../images/ directory and that the paths are correct in your SCSS files.

Once these issues are resolved, your project will be in great shape! Keep up the great work, and feel free to reach out if you have any questions or need further assistance. 🌟

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

Comment on lines 23 to 26
<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 stylesheet link is pointing to an SCSS file (index.scss). Browsers cannot directly interpret SCSS files. You should compile the SCSS file to a CSS file and link to the resulting CSS file instead.

font-family: Roboto, sans-serif;
box-sizing: border-box;
padding: 32px 16px 16px;
background: $white;

Choose a reason for hiding this comment

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

The variable $white is used here. Ensure that it is defined in your SCSS files or imported from another file.

box-sizing: border-box;
padding: 32px 16px 16px;
background: $white;
border: 1px solid $gray-elements;

Choose a reason for hiding this comment

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

The variable $gray-elements is used here. Ensure that it is defined in your SCSS files or imported from another file.

font-weight: 500;
font-size: 12px;
line-height: 18px;
color: $main-accent;

Choose a reason for hiding this comment

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

The variable $main-accent is used here. Ensure that it is defined in your SCSS files or imported from another file.

font-weight: 400;
font-size: 10px;
line-height: 1.4;
color: $secondary;

Choose a reason for hiding this comment

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

The variable $secondary is used here. Ensure that it is defined in your SCSS files or imported from another file.

transition: 0.2s ease-in-out;
border: 1px solid $blue-accent;
&:hover {
color: $blue-accent;

Choose a reason for hiding this comment

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

The variable $blue-accent is used here. Ensure that it is defined in your SCSS files or imported from another file.

border: 1px solid $blue-accent;
&:hover {
color: $blue-accent;
background: $white;

Choose a reason for hiding this comment

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

The variable $white is used here. Ensure that it is defined in your SCSS files or imported from another file.

}

@import 'variables';

Choose a reason for hiding this comment

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

Ensure that the variables.scss file contains all the necessary variable definitions used in your SCSS files, such as $white, $gray-elements, $main-accent, $secondary, and $blue-accent.

height: 16px;
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.

Ensure that the star.svg image exists in the ../images/ directory and the path is correct.


@for $i from 1 through 5 {
&--#{$i} .stars__star:nth-child(-n + #{$i}) {
background-image: url(../images/star-active.svg);

Choose a reason for hiding this comment

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

Ensure that the star-active.svg image exists in the ../images/ directory and the path is correct.

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.


Your code meets the task requirements and constraints, and I'm approving your Pull Request to help you move forward with your learning journey. 🚀

Review Summary:

  • HTML File: The structure is mostly well-formed, but remember to compile your SCSS into CSS, as browsers cannot interpret SCSS directly. This is crucial for the functionality of your webpage.

  • SCSS Files:

    • Ensure all SCSS variables ($white, $gray-elements, $main-accent, $secondary, $blue-accent) are defined in your variables.scss file or imported correctly. This is essential for maintaining styling consistency.
    • Verify that image paths for imac.jpeg, star.svg, and star-active.svg are correct and that the images exist in the specified directory.
  • Configuration Files: Your .linthtmlrc.json is well-configured, and no critical issues were found.

  • Final Conclusion: Your code is in good shape overall, with SCSS nesting and BEM blocks correctly implemented. Ensure to address the critical issues mentioned to avoid similar problems in the future.

Keep up the great work, and use this feedback to refine your skills! 😊

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

Comment on lines 23 to 25
<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 HTML file is linking to an SCSS file (index.scss). Browsers cannot interpret SCSS directly. You need to compile your SCSS into CSS and link the resulting CSS file here.

font-family: Roboto, sans-serif;
box-sizing: border-box;
padding: 32px 16px 16px;
background: $white;

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $white is defined in your variables.scss file or imported correctly. This is crucial for maintaining styling consistency.

box-sizing: border-box;
padding: 32px 16px 16px;
background: $white;
border: 1px solid $gray-elements;

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $gray-elements is defined in your variables.scss file or imported correctly. This is crucial for maintaining styling consistency.

font-weight: 500;
font-size: 12px;
line-height: 18px;
color: $main-accent;

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $main-accent is defined in your variables.scss file or imported correctly. This is crucial for maintaining styling consistency.

font-weight: 400;
font-size: 10px;
line-height: 1.4;
color: $secondary;

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $secondary is defined in your variables.scss file or imported correctly. This is crucial for maintaining styling consistency.

transition: 0.2s ease-in-out;
border: 1px solid $blue-accent;
&:hover {
color: $blue-accent;

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $blue-accent is defined in your variables.scss file or imported correctly. This is crucial for maintaining styling consistency.

border: 1px solid $blue-accent;
&:hover {
color: $blue-accent;
background: $white;

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $white is defined in your variables.scss file or imported correctly. This is crucial for maintaining styling consistency.

&__image {
width: 100%;
height: 134px;
background-image: url(../images/imac.jpeg);

Choose a reason for hiding this comment

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

Verify that the image imac.jpeg exists in the ../images/ directory and that the path is correct in your SCSS file.

height: 16px;
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.

Verify that the image star.svg exists in the ../images/ directory and that the path is correct in your SCSS file.


@for $i from 1 through 5 {
&--#{$i} .stars__star:nth-child(-n + #{$i}) {
background-image: url(../images/star-active.svg);

Choose a reason for hiding this comment

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

Verify that the image star-active.svg exists in the ../images/ directory and that the path is correct in your SCSS file.

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.

Good job, a few changes to fix.

src/index.html Outdated
class="card"
data-qa="card"
>
<div class="card__image"></div>

Choose a reason for hiding this comment

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

Use img tag to display card image instead of doing it with background property

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

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

Looks good.
Do not forget add demo links to PR description in next tasks.

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.

4 participants