-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
base: master
Are you sure you want to change the base?
Develop #2636
Conversation
There was a problem hiding this 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
There was a problem hiding this 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"> |
There was a problem hiding this comment.
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
<section class="recommended section__page" | ||
id="recommended"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check formatting
<h2 class="title title--secondary"> | ||
Recommended | ||
</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check formatting
<img src="./images/picture-1.jpg" class="card__img" | ||
alt="1"> |
There was a problem hiding this comment.
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
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check formatting
@mixin laptop() { | ||
@media (min-width:744px) { | ||
@content | ||
} | ||
} |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
width: 600px; | ||
height: 757px; |
There was a problem hiding this comment.
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
&__img { | ||
height: 600px; | ||
width: 600px; | ||
} |
There was a problem hiding this comment.
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
DEMO LINK
B&O