Header CLS fixes, reduced styles and classes complexity #416
Conversation
| @@ -23,7 +23,7 @@ | |||
| * International Registered Trademark & Property of PrestaShop SA | |||
| *} | |||
|
|
|||
| <div id="_desktop_search" class="order-2 ms-auto"> | |||
| <div id="_desktop_search" class="order-2 ms-auto col-auto px-1 hidden-on-mobile"> | |||
There was a problem hiding this comment.
We wanted to get rid of these hidden-on-* classes but seems like we forgot to remove them, to rely more on bootstrap classes as you specified in the description
|
And here is a croissant as a gift to make you motivated 🥐 😄 |
Oksydan
changed the title
|
@NeOMakinG ready for CR |
| } | ||
|
|
||
| @include media-breakpoint-up(md) { | ||
| padding: $btn-padding-y $btn-padding-x; |
There was a problem hiding this comment.
I guess we don't have any CSS variables to consume there?
There was a problem hiding this comment.
Thanks @NeOMakinG for your review, changes applied.
|
@Oksydan Igor could you please resolve the conflicts? I will merge it after 👍 |
Oksydan
force-pushed
the
HEADER_CLS_FIXES
branch
from
6a6bcd8
to
b50daf4
Compare
|
@Hlavtox branch rebased |
|
🚀🚀🚀 |
|
@Oksydan Could you please check the sign in button bro? The name is not hiding and chrome is applying some useragent styles, because we removed btn and btn-link classes. Thank yooooou. 🙏 |
|
Whoops @Hlavtox. No QA |
BLOCK__ELEMEMT__ELEMENTto justBLOCK_EMLEMENT.It is first part of CLS fixes for header. Also we are reducing some complexity in styles and I would like to relay more on bootstrap components instead of reinventing the wheel with some extra unneeded styles.
It will not resolve whole issue but just some part of it. It will be easier to review code, provide feedback and explain changes if we split it into multiple PR-s.
We are right now showing preview of mobile block for the js loading time, there is no longer jumping stuff around on loading on mobile devices. We will be also making preload components in next stage of CLS fixes after this one will be reviewed and merged.
Comparison (Throttling 3G FAST):
BEFORE:
before.mov
AFTER:
after.mov
There is multiple changes to header structure and we could go even further with that
header-top__rightelement and children inside but I didn't want to make this PR too big. I will be harder to review and make changes according to review..hidden-on-mobileand.hidden-on-desktopclasses has been removed and blocks got proper class replacement for that.One extra note: we shouldn't make more elements than just one in our BEM classes example:
.header__top__right- we got here way to deep, this class has been replaced by.header-top__right.There is a lot of example when BEM can goes wrong https://scalablecss.com/bem-nesting-grandchild-elements/