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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add task solution #4979

wants to merge 1 commit into from

Conversation

RHerheman
Copy link

  • DEMO LINK

  • TEST REPORT LINK

  • [ x ] there 2 BEM blocks card and stars each in their own file

  • [ x ] SCSS Nesting is used for elements, modifiers and pseudo-classes

  • [ x ] SCSS Variables are used for main values and placed in a separate file

  • [ x ] all stars--N modifiers work as expected (Highlight first N stars)

  • [ x ] Code follows all the Code Style Rules ❗️

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 your project! 🎉 Your code meets most of the task requirements and constraints, and it's clear you've put in a lot of effort. Here are some key points to focus on for improvement:

  • SCSS Variables: Ensure all variables like $black, $width, $white, $blue, $grey, and $margin are defined in your variables.scss file or imported correctly. This is crucial for your styles to be applied as expected.

  • Stars Modifiers: While the stars--4 modifier works, make sure all stars--N modifiers are implemented and tested to ensure full functionality.

Your attention to detail with BEM methodology, SCSS nesting, and other requirements is commendable. Keep up the good work and address the mentioned issues to enhance your code quality. Remember, learning from these points will help you grow as a developer. 🚀

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

}

body {
margin: $margin;

Choose a reason for hiding this comment

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

The variable $margin is used here, but it is not defined in this file. Ensure that it is defined in a separate SCSS file or imported correctly.

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

Choose a reason for hiding this comment

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

The variable $black is used here, but it is not defined in this file. Ensure that it is defined in a separate SCSS file or imported correctly.

font-weight: 400;
font-size: 10px;
line-height: 14px;
color: $grey;

Choose a reason for hiding this comment

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

The variable $grey is used here, but it is not defined in this file. Ensure that it is defined in a separate SCSS file or imported correctly.

}

&__price {
width: $width;

Choose a reason for hiding this comment

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

The variable $width is used here, but it is not defined in this file. Ensure that it is defined in a separate SCSS file or imported correctly.

width: $width;
height: 40px;
border-radius: 5px;
background-color: $blue;

Choose a reason for hiding this comment

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

The variable $blue is used here, but it is not defined in this file. Ensure that it is defined in a separate SCSS file or imported correctly.


&:hover {
border: 1px solid $blue;
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, but it is not defined in this file. Ensure that it is defined in a separate SCSS file or imported correctly.

.stars {
display: flex;
justify-content: space-between;
width: $width;

Choose a reason for hiding this comment

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

The variable $width is used here, but it is not defined in this file. Ensure that it is defined in a separate SCSS file or imported correctly.

font-weight: 400;
line-height: 16px;
padding-left: 17px;
color: $black;

Choose a reason for hiding this comment

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

The variable $black is used here, but it is not defined in this file. Ensure that it is defined in a separate SCSS file or imported correctly.

Copy link

@volodymyr-soltys97 volodymyr-soltys97 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 👍
Let's improve your code
This part of the task is incomplete, you need to use styles for stars from the previous task
image
image

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.

This image isn't background, so you need to add image with the img tag

<p class="card__product">Product code: 195434</p>

<div class="stars stars--4">
<div class="stars__image"></div>

Choose a reason for hiding this comment

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

Fix it everywhere

Suggested change
<div class="stars__image"></div>
<div class="stars__star"></div>

</div>

<div class="card__price">
<div class="card__price_text">Price:</div>

Choose a reason for hiding this comment

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

You need to use the semantic tag for text, fix it everywhere

Suggested change
<div class="card__price_text">Price:</div>
<p class="card__price_text">Price:</p>

<div class="card__price_amount">$2,199</div>
</div>

<div class="card__box">

Choose a reason for hiding this comment

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

Redundant wrapper

Suggested change
<div class="card__box">

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