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

Closed
wants to merge 0 commits into from
Closed

add task solution #3951

wants to merge 0 commits into from

Conversation

ogchy
Copy link

@ogchy ogchy commented Jan 6, 2024

Copy link

@SanyaBratashchuk SanyaBratashchuk left a comment

Choose a reason for hiding this comment

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

check font-sizes, colors, intends through the project

src/index.html Outdated
Comment on lines 20 to 26
<div class="card__description">
<h2>APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)</h2>
</div>

<div class="card__product">
<h3>Product code: 195434</h3>
</div>

Choose a reason for hiding this comment

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

Suggested change
<div class="card__description">
<h2>APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)</h2>
</div>
<div class="card__product">
<h3>Product code: 195434</h3>
</div>
<h3 "card__title">APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)</h3>
<p class="card__product">Product code: 195434</p>

src/index.html Outdated

<div class="stars">
<div class="stars__raiting">
<img src=".//images/stars.png" alt="stars">

Choose a reason for hiding this comment

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

image

src/index.html Outdated
Comment on lines 33 to 35
<div class="stars__reviews">
<h4>Reviews: 5</h4>
</div>

Choose a reason for hiding this comment

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

Suggested change
<div class="stars__reviews">
<h4>Reviews: 5</h4>
</div>
<p class="card__reviews">Reviews: 5</p>

Comment on lines 6 to 10
.card {
background-color: #fff;
width: 200px;
height: 408px;
}

Choose a reason for hiding this comment

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

add paddings, border for card

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

Choose a reason for hiding this comment

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

Suggested change
height: 408px;

width: 200px;
height: 408px;
padding: 20px;
}

Choose a reason for hiding this comment

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

Suggested change
}
border-radius: 5px;
border: 1px solid #F3F3F3;
}

background-color: #fff;
width: 200px;
height: 408px;
padding: 20px;

Choose a reason for hiding this comment

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

Suggested change
padding: 20px;
padding: 32px 19px 16px;

.card__image {
width: 160px;
height: 134px;
margin: 32px 20px 40px;

Choose a reason for hiding this comment

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

Suggested change
margin: 32px 20px 40px;
margin-left: 3px;

color: #616070;
}

.stars {

Choose a reason for hiding this comment

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

each BEM block should be in separate file

Comment on lines 47 to 49
.stars--4 .stars__star:nth-child(-n+4) {
background-image: url('/src/images/star-active.svg');
}

Choose a reason for hiding this comment

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

reuse styles from previous task with stars

margin-left: auto;
}

.button {

Choose a reason for hiding this comment

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

there should be just 2 BEM blocks card and stars so button should be an element of card BEM block

Copy link

@l4st1m0za l4st1m0za left a comment

Choose a reason for hiding this comment

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

  1. Use nesting in the SCSS files
  2. As was told before, check all the fonts, font-sizes and coloures of the borders. Everything should be as in the Figma.

@ogchy ogchy requested a review from l4st1m0za January 11, 2024 19:41
Copy link

@l4st1m0za l4st1m0za left a comment

Choose a reason for hiding this comment

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

These are differences from the first test
image

And these are from the second test
image

You need to pay attention to the: font, font-weight, font-size, border-color, width

@ogchy
Copy link
Author

ogchy commented Jan 11, 2024

These are differences from the first test image

And these are from the second test image

You need to pay attention to the: font, font-weight, font-size, border-color, width

image

@ogchy
Copy link
Author

ogchy commented Jan 11, 2024

These are differences from the first test image

And these are from the second test image

You need to pay attention to the: font, font-weight, font-size, border-color, width

ill do it but without button

@ogchy ogchy requested a review from l4st1m0za January 12, 2024 19:44
@ogchy
Copy link
Author

ogchy commented Jan 12, 2024

These are differences from the first test image

And these are from the second test image

You need to pay attention to the: font, font-weight, font-size, border-color, width

can you check

These are differences from the first test image

And these are from the second test image

You need to pay attention to the: font, font-weight, font-size, border-color, width

https://ogchy.github.io/layout_product-cards/ can u check this? something wrong with npm test

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.

Still not fixed from the previous review, you need to fix the failed test
image

Also this part of the task is incomplete
image

src/index.html Outdated
Comment on lines 12 to 18
<div>
<img
src=".//images/imac.jpeg"
alt="imac"
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 div is redundant

Suggested change
<div>
<img
src=".//images/imac.jpeg"
alt="imac"
class="card__image"
>
</div>
<img
src=".//images/imac.jpeg"
alt="imac"
class="card__image"
>

src/index.html Outdated
Comment on lines 50 to 52
<button class="card__button">
BUY
</button>

Choose a reason for hiding this comment

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

button is redundant here

Suggested change
<button class="card__button">
BUY
</button>
BUY

src/index.html Outdated
Comment on lines 26 to 35
<div class="stars stars--4">
<div class="stars__star"></div>

<div class="stars__star"></div>

<div class="stars__star"></div>

<div class="stars__star"></div>

<div class="stars__star"></div>

Choose a reason for hiding this comment

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

Suggested change
<div class="stars stars--4">
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars stars--4">
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>

src/index.html Outdated
Comment on lines 22 to 25
<p class="card__product">Product code: 195434</p>


<div class="card__reviews">

Choose a reason for hiding this comment

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

Suggested change
<p class="card__product">Product code: 195434</p>
<div class="card__reviews">
<p class="card__product">Product code: 195434</p>
<div class="card__reviews">

Comment on lines 37 to 38
font-style: normal;
font-weight: 400;

Choose a reason for hiding this comment

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

Default value, so you can just remove it everywhere

Suggested change
font-style: normal;
font-weight: 400;

}

&__button:hover {
color: #00ACDC;

Choose a reason for hiding this comment

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

Create a variable for repeated color

@ogchy ogchy closed this Jan 25, 2024
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