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

grid: checks for $grid-columns > 0 #30605

Merged
merged 2 commits into from Apr 18, 2020

Conversation

zalog
Copy link
Contributor

@zalog zalog commented Apr 16, 2020

This PR adds:

  • a check for $grid-columns > 0
  • permits us to use to assign $grid-columns: 0; and removing all .col-[number] when needed

@zalog zalog requested a review from a team as a code owner April 16, 2020 14:19
@zalog zalog changed the title checks for $columns grid: checks for $grid-columns > 0 Apr 16, 2020
@MartijnCuppens
Copy link
Member

We already have the $enable-grid-classes variable, which covers this.

@zalog zalog force-pushed the zalog-grid-columns-0 branch 3 times, most recently from 841ad90 to afef2ca Compare April 17, 2020 16:13
@mdo
Copy link
Member

mdo commented Apr 17, 2020

Yeah, and I favor the explicit option of $enable-grid-classes vs something implicit based on a variable's value. Seems safe to close?

@mdo mdo closed this Apr 17, 2020
@zalog
Copy link
Contributor Author

zalog commented Apr 18, 2020

Sorry @mdo, @MartijnCuppens and I, have some slack dm's on this subject and I forgot to log a follow up here.

Here is a use case for a grid version, a minimal one:

$enable-grid-classes: true; // option as is default
$grid-columns: 0; // covered by this PR
$grid-row-columns: 0; // covered by #30606
$grid-breakpoints: (); // covered by #30609

So after this, our grid will look like:
.row {...}
.row > * {...}
.col {...}
.row-cols-auto > * {...}
.col-auto {...}

So no .col-* and .offset-*. This config, but with some $grid-breakpoints of course :) can be a valid choice if we don't need cols number and we just want equals one or auto.

What do you think?

I think that it's a small change for a big opportunity of improving css filesize when our users doesn't need the whole grid :)

@MartijnCuppens
Copy link
Member

It's an edge case, but since it only adds a single if() test and prevents our codebase from crashing if $grid-columns: 0;, this is worth the change.

@MartijnCuppens MartijnCuppens added this to Inbox in v5 via automation Apr 18, 2020
@MartijnCuppens MartijnCuppens added this to Inbox in v4.5.0 via automation Apr 18, 2020
v5 automation moved this from Inbox to Approved Apr 18, 2020
@MartijnCuppens MartijnCuppens merged commit 338ca83 into twbs:master Apr 18, 2020
v5 automation moved this from Approved to Shipped Apr 18, 2020
@zalog zalog deleted the zalog-grid-columns-0 branch April 18, 2020 09:53
MartijnCuppens added a commit that referenced this pull request Apr 18, 2020
grid: checks for `$grid-columns > 0`
@MartijnCuppens MartijnCuppens moved this from Inbox to Cherry-picked in v4.5.0 Apr 18, 2020
XhmikosR pushed a commit that referenced this pull request Apr 21, 2020
grid: checks for `$grid-columns > 0`
XhmikosR pushed a commit that referenced this pull request Apr 28, 2020
grid: checks for `$grid-columns > 0`
@XhmikosR XhmikosR moved this from Cherry-picked to Shipped in v4.5.0 Apr 28, 2020
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
Co-authored-by: Martijn Cuppens <martijn.cuppens@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4.5.0
  
Shipped
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

3 participants