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

Split grid & containers #29146

Merged
merged 1 commit into from Feb 3, 2020
Merged

Conversation

MartijnCuppens
Copy link
Member

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
@MartijnCuppens MartijnCuppens added this to Inbox in v5 via automation Jul 26, 2019
@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

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

@ysds
Copy link
Member

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
Member

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

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

Why not organize the mixin files?

Done

@voltaek voltaek mentioned this pull request Dec 13, 2019
9 tasks
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.

v5 automation moved this from Inbox to Approved Feb 3, 2020
@MartijnCuppens
Copy link
Member Author

:shipit:

@MartijnCuppens MartijnCuppens merged commit 532feab into master Feb 3, 2020
v5 automation moved this from Approved to Shipped Feb 3, 2020
@MartijnCuppens MartijnCuppens deleted the master-split-containers-grid branch February 3, 2020 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

4 participants