-
Notifications
You must be signed in to change notification settings - Fork 487
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
fix(@clayui/css): LPD-47129 Mixins, Improve performance in alerts, ba… #5927
Conversation
$_badge-item-before: map-get($map, badge-item-before); | ||
|
||
@if ($_badge-item-before) { | ||
.badge-item-before { | ||
@include clay-css($_badge-item-before); | ||
} |
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.
This is very interesting, in JS you can do all this inline, you save memory and could have the help of inline cache, but interestingly here it could be faster. Maybe this is faster because you pass an empty value to clay-css
?
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.
You have a point. I'll test in the clay-css mixin and others.
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.
@matuzalemsteles Sass actually takes a good amount of time processing CSS rule-sets. Processing a simple rule-set:
.selector{
color: red;
}
100k times takes 1700ms on my machine. If we wrap it in @if (false){}
it reduces it to 40ms.
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.
@pat270 That's interesting, the bad thing about it is that we need to replicate this rule everywhere.
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.
Yeah it's unfortunate, but we only need to apply it to the mixins to get a big boost in build time.
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.
LGTM
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.
Everything looks good to me!
…dges, and cards
https://liferay.atlassian.net/browse/LPD-47129
This shaved like 4s off
yarn compile
.