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

Develop #2636

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

Develop #2636

wants to merge 40 commits into from

Conversation

bartKarb
Copy link

Copy link

@nataliaklonowska nataliaklonowska left a comment

Choose a reason for hiding this comment

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

don't forget to update demo link

Copy link

@asakevych asakevych left a comment

Choose a reason for hiding this comment

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

fix paddings on sections, fix grid using generic values, add alt to every image and remember it should describe the image

<body class="section">
<header class="section__header header"
id="header">
<div class="header-navigation navigation">

Choose a reason for hiding this comment

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

if you want navigation to in header block it should have class header__navigation -> due to bem it should be block__element--modifier

Comment on lines +110 to +111
<section class="recommended section__page"
id="recommended">

Choose a reason for hiding this comment

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

check formatting

Comment on lines +113 to +115
<h2 class="title title--secondary">
Recommended
</h2>

Choose a reason for hiding this comment

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

check formatting

Comment on lines +119 to +120
<img src="./images/picture-1.jpg" class="card__img"
alt="1">

Choose a reason for hiding this comment

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

check formatting, alt should describe what is at image

Comment on lines +125 to +127
<p class="speaker__paragraph">
Huge sound anywhere – in the home or outdoors. Beosound A5 is a Wi-Fi and Bluetooth speaker with long-lasting battery and wireless charging built in.
</p>

Choose a reason for hiding this comment

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

check formatting

Comment on lines +47 to +51
@mixin laptop() {
@media (min-width:744px) {
@content
}
}

Choose a reason for hiding this comment

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

it's tablet width, change name

gap: 20px;

@include laptop {
grid-template-columns: repeat(6, 80px);

Choose a reason for hiding this comment

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

use 1fr instead of 80px

gap: 20px;

@include laptop {
grid-template-columns: repeat(6, 80px);

Choose a reason for hiding this comment

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

use 1fr instead of 80px

Comment on lines +40 to +41
width: 600px;
height: 757px;

Choose a reason for hiding this comment

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

no need to define sizes using px, it's not responsive, by default it will fit content

Comment on lines +43 to +46
&__img {
height: 600px;
width: 600px;
}

Choose a reason for hiding this comment

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

make image width: 100% and height auto

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