-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Reduce build size by optimizing redundant css #275
base: master
Are you sure you want to change the base?
Conversation
src/css/flexboxgrid.css
Outdated
.col-xs-offset-10, | ||
.col-xs-offset-11, | ||
.col-xs-offset-12 { | ||
[class|="col"] { |
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.
That is not gonna work.
|=
means class attribute starts
with col
.
What if a developer will add additional class in the beginning of the attribute value? Like <div class="something col-sm-..."
?
I would use something like [class*="col-xs]..., [class*="col-sm]
....
But again, even that approach restricted a developer on adding some utils classes like col-xs-whatever
.
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.
About first one - good point. Didn't think about it for some reason. Gonna fix now.
But again, even that approach restricted a developer on adding some utils classes
Well, it doesn't, actually.
[class*="col-"]
would clearly cut it for both usecases, since it would match both the usual col classes and custom utility ones too:
<element class="col-xs-6 col-xs-make-it-very-cool col-lg-make-it-very-cool">
In any case, current waterfall of classes every 100 or so lines does not cut it for the second use-case either, so the only drawback to defining common rules as [class*="col-"]
would be that all similarly-looking classes would fit this selector (which I personally view more like a nice feature).
Note: build size still smaller by a chunk.
@@ -224,6 +224,13 @@ | |||
width: var(--container-sm, 46rem); | |||
} | |||
|
|||
[class*="col-sm"] { |
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.
speaking of reducing bundle size, why you cannot to put styles in one place and separate attributes with ,
?
[class*="col-xs"],
[class*="col-sm"],
[class*="col-md"],
[class*="col-lg"] {
box-sizing: border-box;
flex: 0 0 auto;
padding-right: var(--half-gutter-width, 0.5rem);
padding-left: var(--half-gutter-width, 0.5rem);
}
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 depends on expected behavior.
The way you mentioned, common styles would apply regardless of screen size, whereas in this PR styles still apply in regards to current screen size and xs/sm/md/lg modifiers as it originally was.
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.
I don't know which one is preferred though, so I view both versions as valid.
Description
This change removes unnecessary rules by encapsulating the functionality in a single selector, making sources easier to maintain while also reducing build size.
Check List
instruction : terminal command
grunt
index.html
in a browser & resize to test visual issues