Skip to content
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

.container, .container-fluid should be conditional upon $enable-grid-… #18695

Merged
merged 1 commit into from
Jan 6, 2016
Merged

.container, .container-fluid should be conditional upon $enable-grid-… #18695

merged 1 commit into from
Jan 6, 2016

Conversation

bassjobsen
Copy link
Contributor

…classes

see: #17586

Notice that all code can also wrapped in a single @if $enable-grid-classes {} now.

Code can also be wrapped inside a mixin grid-classes() with the following code in bootstrap.scss:
@import "grid";
@if $enable-grid-classes {
@include grid-classes;
}

And finally notice that the @import "grid"; is not needed at all when you do not need the precompiled grid classes. Removing the import from bootstrap.scss is as simple as setting $enable-grid-classes to false.

@cvrebert cvrebert added this to the v4.0.0-alpha.3 milestone Dec 27, 2015
@cvrebert
Copy link
Collaborator

MSTM. CC: @mdo

@bassjobsen
Copy link
Contributor Author

what does mstm means?

@cvrebert
Copy link
Collaborator

"Makes sense to me"

@bassjobsen
Copy link
Contributor Author

thanks! MSTM :)

2015-12-27 16:11 GMT+01:00 Chris Rebert notifications@github.com:

"Makes sense to me"


Reply to this email directly or view it on GitHub
#18695 (comment).

mdo added a commit that referenced this pull request Jan 6, 2016
.container, .container-fluid should be conditional upon $enable-grid-…
@mdo mdo merged commit 3dd08d6 into twbs:v4-dev Jan 6, 2016
@mdo
Copy link
Member

mdo commented Jan 6, 2016

Snap, thanks!

@mdo mdo mentioned this pull request Jan 6, 2016
@dalabarge
Copy link

I just had a use case where I do not rely on the grid classes but do use the grid mixins and expected the container classes to exist. With an update to this commit, it broke. To turn on the grid would bloat my CSS output with a lot of unused classes just so I can get the container classes enabled. I do use the grid, just not the classes. I could foresee also not even needing the grid but perhaps needing the containment of the container so that I get more predictable layouts at defined breaking points.

Not sure this inclusion really make sense as the container doesn't necessarily connect to the grid so making them dependent on each other seems unnecessary coupling. Another config option seems in order howbeit then we are getting into config overhead land. Maybe $enable-container-classes or perhaps separate the grid classes from the concept of grid entirely so that classes are included with $enable-grid-classes (turns on all .col-* and .row classes) while $enable-grid turns on all related classes and mixins such as .container and .container-fluid. You could then have $enable-grid == true and $enable-grid-classes == false to achieve what this PR permits while still giving my use case a simple solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants