Skip to content

Split grid & containers#29146

Merged
MartijnCuppens merged 1 commit intomasterfrom
master-split-containers-grid
Feb 3, 2020
Merged

Split grid & containers#29146
MartijnCuppens merged 1 commit intomasterfrom
master-split-containers-grid

Conversation

@MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Jul 26, 2019

The grid and containers are 2 different components, let's split them into different scss files.

@MartijnCuppens MartijnCuppens requested a review from a team as a code owner July 26, 2019 18:06
@mdo
Copy link
Member

mdo commented Jul 26, 2019

Why wouldn't we continue to wrap all this in a the $enable-grid-classes?

@MartijnCuppens
Copy link
Member Author

MartijnCuppens commented Jul 26, 2019 •
edited
Loading

The containers and grid system are two different things IMO, they can be used independent.

@ysds
Copy link
Contributor

ysds commented Jul 26, 2019

Why not organize the mixin files?

@mdo
Copy link
Member

mdo commented Jul 26, 2019

Containers are required for grid system though, too, because of the horizontal padding.

@ysds
Copy link
Contributor

ysds commented Jul 27, 2019

I agree with @MartijnCuppens. What is required is not a .container, but a wrapper with left and right paddings.

@MartijnCuppens
Copy link
Member Author

MartijnCuppens commented Jul 27, 2019

Containers are required for grid system though, too, because of the horizontal padding.

Valid point. However, the $enable-grid-classes name might be confusing. I wouldn't expect the containers from not working anymore when I disable it. If we want a single variable to enable the grid classes and container classes it might be clearer to rename it to $enable-grid-and-container-classes.

Or maybe we should introduce a new $enable-container-classes variable?

@MartijnCuppens MartijnCuppens force-pushed the master-split-containers-grid branch 2 times, most recently from 0e752e9 to 1fab174 Compare August 14, 2019 15:16
@MartijnCuppens
Copy link
Member Author

MartijnCuppens commented Aug 14, 2019

Why not organize the mixin files?

Done

XhmikosR
XhmikosR reviewed Aug 14, 2019
View reviewed changes
@voltaek voltaek mentioned this pull request Dec 13, 2019
Merged
9 tasks
mdo
mdo approved these changes Feb 3, 2020
View reviewed changes
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revisiting this, I'm fine to do the split here and give folks more granular control. @MartijnCuppens let me know if you want to handle in the first alpha or subsequent release.

@MartijnCuppens MartijnCuppens force-pushed the master-split-containers-grid branch from 74fa929 to e49ef44 Compare February 3, 2020 19:39
@MartijnCuppens MartijnCuppens force-pushed the master-split-containers-grid branch from e49ef44 to 8ac8239 Compare February 3, 2020 19:44
@MartijnCuppens
Copy link
Member Author

MartijnCuppens commented Feb 3, 2020

:shipit:

@MartijnCuppens MartijnCuppens merged commit 532feab into master Feb 3, 2020
@MartijnCuppens MartijnCuppens deleted the master-split-containers-grid branch February 3, 2020 20:02
@mdo mdo mentioned this pull request Sep 14, 2020
Merged
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
@MartijnCuppens @olsza
Split grid & containers (twbs#29146)
78d1d78
@dmetzner dmetzner mentioned this pull request Sep 24, 2021
Merged
@CloCkWeRX CloCkWeRX mentioned this pull request Jan 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XhmikosR XhmikosR XhmikosR left review comments

@mdo mdo mdo approved these changes

Assignees

No one assigned

Labels

css no-backport-to-v4 v5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MartijnCuppens @mdo @ysds @XhmikosR