Develop #978
Develop #978
Conversation
|
| <a | ||
| href="#menu" | ||
| class="navigation__item navigation__item--menu" | ||
| > | ||
| <img | ||
| src="./images/Vector.svg" | ||
| alt="menu" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
for desktop you should replace burger menu with links (you can make use of display: none)
| <div class="header__slider-block"> | ||
| <img | ||
| src="../src/images/slider/slide-img-1.jpg" | ||
| alt="slide" | ||
| class="header__slider-image" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
you should center position of this image and only clip sides (you can change it for the one used in design to better see differences)
src/styles/blocks/navigation.scss
Outdated
| &--menu img { | ||
| vertical-align: bottom; | ||
| width: 18px; | ||
| height: 8px; | ||
| } |
There was a problem hiding this comment.
don't use tags to improve consistent with specificity ;)
|
I'm trying to make a responsive 'header'. Please check my code and give some advice. |
|
Please check my code and give some advice. |
src/styles/blocks/list.scss
Outdated
| .list { | ||
| display: none; | ||
|
|
||
| @include for-desktop { | ||
| display: flex; | ||
| gap: 48px | ||
| } | ||
|
|
||
| @include for-large { | ||
| display: flex; | ||
| gap: 48px; | ||
| } |
There was a problem hiding this comment.
There is a significant amount of repeated code.
src/styles/utils/_mixins.scss
Outdated
| @mixin for-tablet { | ||
| @media (min-width: 640px) and (max-width: 1024px) { | ||
| @content; | ||
| } | ||
| } | ||
|
|
||
| @mixin for-desktop { | ||
| @media (min-width: 1024px) and (max-width: 1600px) { | ||
| @content; | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe that your approach of using both min-width and max-width in media queries is excessive for this design. It results in a lot of repeated styles.
| @include for-desktop { | ||
| width: calc(50% - (20px) / 2); | ||
| left: auto; | ||
| height: 680px; | ||
| border-radius: 30px 0; | ||
| } | ||
|
|
||
| @include for-large { | ||
| width: calc(50% - ((30px) / 2) - (30px + 68px)); | ||
| left: auto; | ||
| height: 680px; | ||
| border-radius: 30px 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
A border radius should only be applied to the top-left corner of the image for screen widths between 1024px and 1600px.
|
Pay attention to the services section. Perhaps there are options to simplify the code. Hope you can help |
src/index.html
Outdated
| > | ||
| <p class="services__section-name">Services</p> | ||
|
|
||
| <h2 class="main__title services__title"> |
There was a problem hiding this comment.
You broke BEM rule here: The parent block element cannot be used inside another, nested block.
src/styles/blocks/services.scss
Outdated
| @include for-tablet { | ||
| padding: 148px 41px 48px; | ||
|
|
||
| grid-template-rows: auto auto 51px 132px 162px 132px 162px 132px; |
There was a problem hiding this comment.
Whoa, that's a weird value! 🤯 I think you don't have to do such complex calculations with rows. And just declare that it should be in x row. Then just tweak its position with margin.
|
Please check my mobile version and give some advices |
I'm trying to make a responsive 'header'. Please check my code and give some advice.
DEMO LINK.