Develop #1
Develop #1
Conversation
| <div class="main-header__wrapper"> | ||
| <div class="main-header__branding"> | ||
| <a href="#" class="main-header__branding-link"> | ||
| <img class="main-header__logo" src="dist/images/logo.svg" alt="logo"> |
There was a problem hiding this comment.
Recommended to have width and height attributes.
| <ul class="main-navigation__list"> | ||
| <li class="main-navigation__item"> | ||
| <a href="#" class="main-navigation__item-link">About us</a> | ||
| </li> | ||
| <li class="main-navigation__item main-navigation__item_has-children"> | ||
| <a href="#" class="main-navigation__item-link">Membership</a> | ||
| <span class="sub-menu-indicator"></span> |
There was a problem hiding this comment.
Submenu indicator stands far from a link. This cause behaviour that sub-menu collapse/expand continuously.
The triggering area should be consistent. Either the whole li (if the indicator is outside the link) or the link (if the indicator is a pseudo-element of a link).
| max-width: 540px; | ||
| } | ||
| } | ||
| @media screen and (min-width: 768px){ |
|
|
||
| } | ||
| @media screen and (max-width: 768px){ | ||
| #body h1 { |
There was a problem hiding this comment.
Recommended not using ID for styles applying. Classes work better.
| right: -18px; | ||
| width: 7px; | ||
| height: 7px; | ||
| background: transparent; |
There was a problem hiding this comment.
Recommended to use non-words color definitions (e.g. rgba). In case there will be a mixin with function to lighten or darken a color there could be problems. Please note that lighten and darken functions itself work fine, but in some cases, they could fail.
|
|
||
| &::after { | ||
| right: calc(50% - 5px); | ||
| top: calc(50% - 7px); |
There was a problem hiding this comment.
For centering it's recommended using: {top: 50%; transform: translateY(-50%);}
| z-index: 100; | ||
| &__list { | ||
| position: absolute; | ||
| right: -400px; |
There was a problem hiding this comment.
Recommend to either fix menu width to the same value as a shift {width: 400px; right: -400px;} or applying other techniques (e.g. {left: 200vw}, .wrapper {overflow: hidden} .inner {left: 100%;}).
| &__sub-menu { | ||
| position: static; | ||
| box-shadow: none; | ||
| max-height: 0; |
There was a problem hiding this comment.
Seems that this property isn't necessary.
| &::after { | ||
| border-color: $main-link-color; | ||
| top: calc(50% - 3px); | ||
| transform: rotate(-136deg); |
| } | ||
| } | ||
| .main-navigation__sub-menu { | ||
| max-height: 500px; |
There was a problem hiding this comment.
Recommend to use either viewport based limit (100vh) or leave limitless.
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| height: 28px; |
There was a problem hiding this comment.
The size of the trigger is controlled by the height of one wrapper and the width of the other.
Here is a suggestion:
.trigger-wrapper {width: 32px; height: 0; padding-top: 100%;} /Drawing a scalable square/
.trigger-mark {width: 100%; height: 12.5%;} /* Height is 32px, 12.5% of that is 4px /
.trigger-mark::before, .trigger-mark::after {width: 100%; height: 100%;} / Since trigger-mark is absolute positioned width and height will be inherited */
This approaches allows to control the whole sizing using one property (width of the wrapper).
| </div> | ||
| <div class="agents__tfoot agents__footer table"> | ||
| <div class="agents__tr tr"> | ||
| <div class="agents__th th">Total agents: 28 </div> |
There was a problem hiding this comment.
Texts and values are better to wrap separately. That helps a lot for JS actions (clone, insert, get value) where you won't need any parsing.
| const tableBody = document.querySelector('.agents__body') | ||
| const tableFooter = document.querySelector('.agents__footer') | ||
|
|
||
| window.addEventListener('resize', setWidth) |
There was a problem hiding this comment.
Probably since there is an adaptive approach was used (content width changed iteratively) there is no need to have a resize listener. Tracking resize could be replaced with the following - https://developer.mozilla.org/en-US/docs/Web/API/Window/matchMedia
There was a problem hiding this comment.
Or at least add listener only when content size is really responsive.
|
|
No description provided.