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 Container Rework #10371

Merged
merged 3 commits into from
Jul 14, 2017
Merged

Grid Container Rework #10371

merged 3 commits into from
Jul 14, 2017

Conversation

brettsmason
Copy link
Contributor

First take on the reworked grid container. Updates docs to show usage as well as modified visual test to show this visually.

To confirm what this does:

  • reworks grid-container to have padding by default
  • removes grid-container-padded but use its variables for the container padding
  • adds grid-container-fluid and grid-container-full
  • Makes grid-padding-x have negative margins if within grid-container/grid-container-fluid to properly line up with margin grids

This should get some solid testing before merging just to make sure this is the behaviour we want, but from the visual tests it works great.

… as well as modified visual test to show this visually.
@IamManchanda
Copy link
Contributor

@brettsmason In v6, its

<button type="button" class="button primary">Primary</button>

In v7 (ofcourse with optional namespace and healthchecks) its gonna be

<button type="button" class="button button--primary">Primary</button>

So why not follow the same pattern with full & fluid and lets go with this below in 6

<div class="grid-container full"></div>

and change like this in 7

<div class="grid-container grid-container--full"></div>

@brettsmason
Copy link
Contributor Author

brettsmason commented Jul 10, 2017

@IamManchanda Yeah that's what I was thinking. I'll change that later.

My one question regarding the full class: It didn't make any sense to me to hide the left/right padding using full for a padding grid so at the moment this only applies to margin grids.

If you take a look at the visual test you will see what I mean. Do you think this makes sense/is correct?

@kball
Copy link
Contributor

kball commented Jul 11, 2017

On a readthrough, this looks good to me.

Copy link
Contributor

@IamManchanda IamManchanda left a comment

Choose a reason for hiding this comment

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

LGTM... Merging!

@IamManchanda IamManchanda merged commit b0e663f into develop Jul 14, 2017
@IamManchanda
Copy link
Contributor

The docs copy is updated ... Awesome work again @brettsmason 😉

@OndrejVasicek
Copy link
Contributor

I'm testing the 6.4.2 now and I don't get the reason for class="grid-container full". It’s the same like not using any container? So why is there a need for such a class? If I don’t want full width and any padding, I’ll just won’t use any grid. Right?

To be honest I don’t even understand why there is the class fluid. If I want some padding in my layout around the full grid-x, I’ll just create some block with $global-padding rather than using this container with strange padding and saying “I’m the full width”, which is the default state for grid without container.
Am I missing something?

@brettsmason
Copy link
Contributor Author

@OndrejVasicek Here's a CodePen to illustrate the differences: https://codepen.io/brettsmason/pen/RZjdKo

You are correct in that you don't really need a container if you are going for the full example, its just there for consistency really (and if we ever move to a left only gutter it will be useful to solve the overlap issue).

The padding is there as now the grid container adds padding to margin and padding grids. This now means you can use both gutter types in the same design as the columns line up correctly (see in the top 2 examples they don't). Prior to 6.4.2 if using padding gutters on one element and margin gutters on another, the columns wouldn't be in line with each other.

Hope that makes sense.

@OndrejVasicek
Copy link
Contributor

Thank you Brett. It makes sense but it raises new question – I thought that one of the reasons we have now padding and margin gutters is the different behaviour on the edges (margin goes to the edge, padding have some padding). But now if I use container, it behaves the same. What’s the difference between padding and margin gutters in this case? When I should use padding and when margin? It seems like the existence of one of them is useless in case of using container.

Or was there a reason why it aligns the edges on the same places when using container?

@JarvisH
Copy link

JarvisH commented Sep 14, 2017

@OndrejVasicek

Just to add my two cents: the difference in padding/margin inside a grid-container would allow for colored backgrounds on padded cells, with contents still aligned to margin cells. Like this: Codepen

@colin-marshall colin-marshall deleted the grid-container-rework branch January 19, 2018 17:58
stuartstarrs added a commit to waqastudios/inti-foundation that referenced this pull request Apr 19, 2018
stuartstarrs added a commit to waqastudios/inti-kitchen-sink that referenced this pull request Apr 19, 2018
stuartstarrs added a commit to waqastudios/inti-acf-starter that referenced this pull request Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants