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

XY Grid - Feedback and Discussion #10141

Closed
brettsmason opened this issue Jun 5, 2017 · 67 comments
Closed

XY Grid - Feedback and Discussion #10141

brettsmason opened this issue Jun 5, 2017 · 67 comments

Comments

@brettsmason
Copy link
Contributor

brettsmason commented Jun 5, 2017

Now that the new XY Grid has been merged and is included in the RC lets keep all discussion surrounding this in one place, and gather as much feedback to improve it as much as possible before the full release.

You’ll find a Codepen to the new grid here:
https://codepen.io/ZURBFoundation/pen/owvqYp?editors=1000

And the docs:
http://foundation.zurb.com/sites/6.4.0-rc1/docs/xy-grid.html

For previous discussion history see:

@brettsmason
Copy link
Contributor Author

brettsmason commented Jun 5, 2017

Currently the plan is to rename .margin/padding-gutters classes to .grid-margin/padding classes.
However feedback on how we can make this more flexible (eg allow padding to be added on all 4 sides, margin on bottom too etc) would be welcomed!

@rafibomb
Copy link
Member

rafibomb commented Jun 5, 2017

Padding to create vertical spacing in the grid - my opinion is that padding as a "gutter" might now be obsolete so combining the .grid-margin with .grid-padding might be the way to achieve this - using the $global-padding variable

@oxyc
Copy link

oxyc commented Jun 7, 2017

Is this a bug? https://codepen.io/anon/pen/owjYyR?editors=1100. Seems something isn't getting overridden correctly as the grid overflows the container.

<div class="grid-y margin-gutters">
  <div class="cell">cell</div>
  <div class="cell">cell</div>
</div>

Also, would be great if this would add vertical gutters only between the cells (not above or below the grid itself, just like grid-x). I often use such a pattern to separate teasers in a content listing where I dont want any trailing whitespace.

@kball
Copy link
Contributor

kball commented Jun 7, 2017

@oxyc can you specify browser? It looks fine in Chrome, but I'm seeing an issue in Safari that looks like this:

xy_grid_gutters___foundation_6_4_docs

@brettsmason can you look into it?

@oxyc
Copy link

oxyc commented Jun 7, 2017

In your screenshot you can see that the cells background color is not aligned with the heading, which I think it should be.

@kball
Copy link
Contributor

kball commented Jun 7, 2017

Ah, yes, I see it. The left-right negative margin from margin gutters is getting applied even though this is a vertical grid. Good catch!

@kball
Copy link
Contributor

kball commented Jun 7, 2017

@oxyc with regards to the vertical gutters not applying on the outside, I believe that actually is working but the h4 has a margin-bottom that is offsetting. Can you verify?

@oxyc
Copy link

oxyc commented Jun 7, 2017

Another common pattern I use is a combination of grid-x andgrid-y. Basically it would add gutters on all sides of the cells, but still align the cell content with the container. This is useful with block grids Demo https://codepen.io/anon/pen/QgjGRO?editors=1100

Edit, this was very difficult to create with padding grids, a pretty easy fix with margins!

@oxyc
Copy link

oxyc commented Jun 7, 2017

@oxyc with regards to the vertical gutters not applying on the outside, I believe that actually is working but the h4 has a margin-bottom that is offsetting. Can you verify?

Yes, sorry, you're correct.

@kball
Copy link
Contributor

kball commented Jun 7, 2017

Another common pattern I use is a combination of grid-x and the way I described grid-y above. Basically it would add gutters on all sides of the cells, but still align the cell content with the container. Demo https://codepen.io/anon/pen/QgjGRO?editors=1100

I think this is what @brettsmason @andycochran, and I have been going back and forth about in with regards to positioning gutters on multiple sides in #10017

Question for you - In this situation how would you expect the gutters to behave on the bottom of the container? Would you expect the grid to be flush with the bottom, or have a gutter beneath it?

@oxyc
Copy link

oxyc commented Jun 7, 2017

Question for you - In this situation how would you expect the gutters to behave on the bottom of the container? Would you expect the grid to be flush with the bottom, or have a gutter beneath it?

I would not expect a gutter. For example I might want to wrap the grid in a <div class="padding-1 bg-primary"></div> where I expect the perceived padding to apply equally all around the grid. If there was a gutter below the grid, the vertical spacing wouldn't match the horizontal. Demo of how I would want it to work https://codepen.io/anon/pen/YQyNEp?editors=1100 (update: added an example with padding-vertical as well)

@brettsmason
Copy link
Contributor Author

@oxyc Thanks so much for your valuable feedback! I'm going to take a look at some changes this evening to address this.

@arjendejong12
Copy link

What are the XY grid equivalents of the Sass mixins @include flex-grid-row() and @include flex-grid-columns()?

@brettsmason
Copy link
Contributor Author

@arjendejong12 Currently the mixins are xy-grid (https://github.com/zurb/foundation-sites/blob/develop/scss/xy-grid/_grid.scss#L23) and xy-cell (https://github.com/zurb/foundation-sites/blob/develop/scss/xy-grid/_cell.scss#L100). With the complexity of the class requirement with the new grid, the mixins have been problematic and I've struggled to find users who use mixins to get feedback.

If you use mixins I'd really appreciate if you are able to give them a try and let me know how you get on!

@arjendejong12
Copy link

arjendejong12 commented Jun 10, 2017

Thank you, it seems I was right about picking those mixins. I was confused by the documentation about xy-cell($size) only accepting full, auto, and shrink, while I was looking for xy-cell() to provide me with the number of columns I'd want, just like flex-grid-columns(). Luckily, it seems xy-cell() also accepts numbers. So it works now and I also tried to change xy-cell($gutter-type) to padding, which gave expected results. Props for making these mixins! Maybe you could edit the documentation to include the possibility to add numbers to xy-cell($size), because for me it was confusing whether it was possible or not. I also think you should move these two mixins to top because they are the most important ones.

A personal wish for these mixins would be to add a map of breakpoint column sizes, something like this: xy-cell($breakpoint: (small: 12, medium: 6, large: 8)). This makes it easier for me to declare various column sizes in one go, instead of using the breakpoint() mixin, but I can see that this might be hard to code.

@JeremyEnglert
Copy link

I think we need to add some docs about cell-block and cell-block container. There's information about the mixins, but it may deserve it's own section in the XY docs.

@brettsmason
Copy link
Contributor Author

@arjendejong12 Thanks again for the feedback - I'll definitely get those docs fixes added.
I also created a PR with a new mixin: xy-cell-breakpoints based on your suggestion. I decided to add it as a new mixin for now, though I'm not sure if it should be part of the xy-cell mixin or not.

@kball what control do we have over mixin order? I'm assuming its based on the order within the partial? I agree it might be good to change the order to one that's more relevant to the end user.

@brettsmason
Copy link
Contributor Author

@JeremyEnglert Agreed - @kball can you help me out with that one?

@kball
Copy link
Contributor

kball commented Jun 12, 2017

@brettsmason you mean in the docs? I believe that is correct, order within the partial and probably order of files in the front matter in the docs as well (I believe you can do an array not just a wildcard)

@JeremyEnglert yes will see what I can put together on cell-block and cell-block-container

@brettsmason
Copy link
Contributor Author

Also noticed today whilst trying out mixins that xy-cell-layout is going to need the responsive treatment, just like the PR for xy-cell-breakpoints. If we can work out how to code this then it will apply to a few of the mixins. Some of the mixins may even be better merged. Don't have any time until Friday to look at anything now though unfortunately.

@oxyc
Copy link

oxyc commented Jun 14, 2017

A few notes while I'm building a site with the new grid:

  1. First off I love the margin grid as it's way easier to align edges without having the side gutters on the grid itself. With the old grid I had to wrap basically every piece of content on the page in a grid to get the content to align. One consequence of having no padding on the outer sides of the grid is that once your viewport becomes smaller than $global-width (assuming you have a wrapping grid-container), the content hugs the viewport edge. My own fix was to add:

    .grid-container {
      // Add a gutter between the grid and the viewport.
      @include xy-gutters($gutters: $grid-padding-gutters, $gutter-type: padding);
    }

    I'm still learning to use the grid but I dont imagine I will ever nest grid-containers, I'm thinking I'll only use them for the main layout of the page. Items that I do want to flex to the viewport sides, such as hero banners, are never grids, so I don't think it's going to cause issues.
    Might this be useful to have as a variable $grid-container-gutter?

  2. I'm still struggling with proper a proper sticky footer component. I was hoping the grid-y would be usable for this but seems it's not.

  3. I often want the content of a cell to take up as much space as possible, so that everything becomes equal height without resorting to equalizer. Not sure how to solve this in a good way yet, for now I'm enabling it globally on all cells to see what happens and try and fix it once I know the downsides. Some designs would obviously be hurt by this so I'll add it as a separate class after having tested it globally. A helper would be useful, in my opinion it should be on a grid basis, rather than on a per cell basis.

@brettsmason
Copy link
Contributor Author

@oxyc thanks for your continued feedback, I really appreciate it! To answer your points:

  1. I've been wondering about how to handle this too. The trouble with adding padding around the .grid-container is the grid wont align with items that arent in a .grid-container. An example is if I wanted a full width grid of images and dont want any padding on the sides. I think adding padding could work, but it should be removed on screens larger than $global-width? Need some other opinions on this one.

  2. What do you mean by a sticky footer - do you have an example? It might be possible to do something with .grid-frame - pretty much creating an app style layout with scrollable panels.

  3. I see you found .align-stretch - I'm working on sorting out this issue at the moment so that will be the way to go to achieve this.

@oxyc
Copy link

oxyc commented Jun 14, 2017

I've been wondering about how to handle this too. The trouble with adding padding around the .grid-container is the grid wont align with items that arent in a .grid-container. An example is if I wanted a full width grid of images and dont want any padding on the sides. I think adding padding could work, but it should be removed on screens larger than $global-width? Need some other opinions on this one.

True. Your suggestion would work for me but I do see the possibility for cases where user want text content to have a padding, while grids of images shouldn't. Both types of grids might still have use for the max-width of grid-container though.

  1. What do you mean by a sticky footer - do you have an example? It might be possible to do something with .grid-frame - pretty much creating an app style layout with scrollable panels.

There are two primary use cases. First is the entire page layout where I want the header and the footer to shrink while the content area in between behaves like auto except that it will grow past the available height if the content takes up more space than is available.

Second use case would be teasers within a grid that uses align-stretch. Same deal, the title and the footer (eg read more) should shrink so that they're aligned with the edges of the cell, while the content takes up whatever space is left, but still affects the height of all other cells in the row.

@brettsmason
Copy link
Contributor Author

@kball I've been having a look at the docs and trying to update the order the mixins are listed.
I've put:

sass:
  - 'scss/xy-grid/_grid.scss'
  - 'scss/xy-grid/_cell.scss'

Into the docs partial, but its still listing cell mixins first, so I'm assuming its doing it alphabetically.
Is there any way around this?

@kball
Copy link
Contributor

kball commented Jun 28, 2017

Hi @marvinhuebner - for a padding grid, you can actually combine the container with the grid... so something like

 <div class="grid-x grid-container grid-padding-x">
 </div>

but for margin it needs to be separate, which was one of the primary reasons for separating that out into its' own class.

For full width cells, not sure exactly what the recommendation would be there... @brettsmason any thoughts?

@IamManchanda
Copy link
Contributor

IamManchanda commented Jun 28, 2017

This is enough i guess for me

<div class="grid-x grid-container grid-padding-x">
 <div class="cell">

 </div> 
</div>

marvinhuebner added a commit to marvinhuebner/foundation-sites that referenced this issue Jun 29, 2017
save some markup if you jave just a single cell by compining .grid-container, .grid-x and .grid-padding-x

see: foundation#10141 (comment)
@brettsmason
Copy link
Contributor Author

@kball sorry for the late reply.

Personally I never understood the need for combining a row and column - they are 2 separate elements and should stay like that in my opinion.

However what @IamManchanda pointed out will of course work. The grid should be modular enough to be able to combine classes for the desired behavior :)

Alternatively if using Sass you could change the grid container padding max width and have it output padding on all sizes, and use that (then no need for cell). However this isn't really a grid, it's just a contained element with padding.

Maybe we need a gutter utility class that can be used to add responsive padding to any element?

@oxyc
Copy link

oxyc commented Jun 30, 2017

Maybe we need a gutter utility class that can be used to add responsive padding to any element?

I used to have that in my projects but in the end it caused more harm because i was using it as a prototyping utility and when the project changed we easily ignored making it into a real grid and gutters would be applied twice.

Nowadays I always wrap it as a proper grid if it could potentially become a grid in the future. For components where I need gutters to space out content from the container (usually containers with a background color or an image), I use named placeholders to add spacing. https://github.com/generoi/sage/blob/genero/resources/assets/styles/utils/_spacings.scss

@brgrz
Copy link

brgrz commented Jul 23, 2017

There's an issue with .grid-frame and .cell-block-y which don't get height: 100% (they only have the max-height: 100% set), which kinda defeats the point of a grid frame/app frame. The issue is that if the content within .cell-block-y doesn't overflow then it never hits the 100% height, which, if you have background applied results in columns that are too short and gives weird look.

The issue can be demonstrated if you delete some paragraphs from the official docs grid frame example. I'm attaching the photo for you to visualize what I mean:

xy-grid-frame-height

Imho most users will require those columns to take up 100%, especially when background is applied.

@brettsmason
Copy link
Contributor Author

@brgrz Thanks for reporting this. Is there any way you could open that as a new issue? I think this thread should be closed now as the grid is live and its a little hard to keep track of 😄

@brgrz
Copy link

brgrz commented Jul 24, 2017

sure #10457

@HansUXdev
Copy link

Serious question. But why has the XY Grid been included by default considering it literally breaks everything?
Every template, every building-block, most components. Everything. Broken.

@ableslayer
Copy link

XY Grid + the equalizer doesn't seem to work around anymore. Is there a built in feature of XY Grid that can solve the issue of multiple items to have equal heights?

@heyflo
Copy link

heyflo commented Sep 4, 2017

A simple question but I seem to struggle on this, why is there no mixin to just change the xy-cell size ?
For example I have this :

li {
    @include xy-cell(6, $gutter-type: padding, $gutter-output: false);
    margin-bottom: rem-calc(5);
    text-align: left;
}

And I just want to change the xy-cell($size), to 4, instead of 6 for large breakpoint so I'm using this :

@include breakpoint(large) {
  @include xy-cell(4);
}

But this override the parameters I have passed above, why is there no specific mixin to just target the cell-size ? There is a function but I can't find any specific usability ^^

Thanks!

@sveggiani
Copy link

sveggiani commented Sep 6, 2017

I'm having some problems making a grid "semantically" using mixins. I've posted this in Foundation forums and found a workoaround already.

http://foundation.zurb.com/forum/posts/54127-i-cant-reproduce-simple-xy-grid-with-mixins

Could someone please explain if this could be made using only "official" mixins? Am I doing something wrong? Thanks!

@brettsmason
Copy link
Contributor Author

@sveggiani are you able to link to a live link of your code? Not entirely sure from looking at the forum thread what you are trying to achieve. Everything will be possible with mixins, I just need a better idea of what you need to do in order to help.

@sveggiani
Copy link

@brettsmason Hi, thanks for your response.

What I'm trying to achieve is to create a simple grid like this:

<!-- XY Grid clases (Works!) -->
<div class="grid-x grid-margin-x">
  <div class="cell small-6">A</div>
  <div class="cell small-6">B</div>
</div>

I would like reproduce that exact grid layout without using Foundation classes in markup but mixins:

<div class="item-row">
  <div class="item-row__cell">A</div>
  <div class="item-row__cell">B</div>
</div>
.item-row {
  @include xy-grid;
}

.item-row__cell {
  @include xy-cell(6);
}

But I'm not being able to express with mixins what the .grid-margin-x does. The only way I've found to achieve this is by extending that class into the row container styles:

.item-row {
  @include xy-grid;
  @extend .grid-margin-x; // <---- this!
}

I'm sorry I can't point you to a live example because I'm not being able to include Foundation SASS mixins in Codepen or similar tools.

Thank you very much.

@brettsmason
Copy link
Contributor Author

@sveggiani sorry for the delay.
So it looks like there might be a small bug with your use case - the base flex properties aren't being applied if you don't use xy-cell with full width.

Therefore for now, you would need to do this:

.item-row {
  @include xy-grid;
}

.item-row__cell {
  @include xy-cell-base;
  @include xy-cell(6);
}

@kball looks like an issue here: https://github.com/zurb/foundation-sites/blob/develop/scss/xy-grid/_cell.scss#L123 however xy-cell-base isn't setup to work with a number: https://github.com/zurb/foundation-sites/blob/develop/scss/xy-grid/_cell.scss#L48

@heyflo
Copy link

heyflo commented Sep 7, 2017

@brettsmason I am using this code to achieve grid-margin-x :

.item-row {
  @include xy-grid;
  @include xy-gutters($negative: true);
}

.item-row__cell {
  @include xy-cell(6);
}

What about my issue above with the lack of xy-cell-size mixin ?

@sveggiani
Copy link

@brettsmason I've made some tests and the solution from @heyflo is the right for my case. Using @include xy-cell-base doens't solve the problem with margins in the container.
Thank you both!

@heyflo
Copy link

heyflo commented Sep 8, 2017

@sveggiani no problem 👍
In my case if you have any idea why there is no mixin such as grid-column-size for the XY Grid to just redefine the size of a cell depending on the breakpoint (see : #10141 (comment)), or if you have any idea on how to achieve this without redefining the whole xy-cell, I'll be thanksful !

@brettsmason
Copy link
Contributor Author

@heyflo Sorry for missing your question - I'm taking a small break from contributing to Foundation at the moment so am not very active.

So to answer your question - I started off with having a xy-cell-size mixin to do what you mentioned. However with the introduction of the margin option things got a lot more complicated, as the gutter is then included in the size so it would have to be passed to the mixin too.

Question - are you using just padding gutters? If so maybe an option to change the default for the mixin, or a mixin to wrap code to set some defaults so you dont have to keep setting them might be a good idea.

With your example, you woud currently need to write:

li {
    @include xy-cell(6, $gutter-type: padding, $gutter-output: false);
    margin-bottom: rem-calc(5);
    text-align: left;

        @include breakpoint(large) {
            @include xy-cell(4, $gutter-type: padding, $gutter-output: false)
        }
}

To be honest the mixins do need a lot of work and probably a rework. They are fairly biased towards using margin gutters at the moment because at the time its what people seemed to be vocal about.

@kball Do you have any thoughts on how to make this scenario easier? I think we really need to completely redo the mixins at some point...

@heyflo
Copy link

heyflo commented Sep 11, 2017

@brettsmason Thanks for answering. That is what I am already using, I figured out there was no other solution. To point out what I wanted to do intially so it does make sense, I wanted to have some cells to have the full width (and not the calc(% - gutters)), so in order to do that I had to set the $gutter-type to padding and turn the output to false.

With some tests, I figured out that only having $gutter-output: false, doesn't remove the calc(% - gutters) for the width size, something I don't really understand. If you don't want gutters then I think the width should be a clear % and not a calc(% - gutters).

@jmartsch
Copy link

jmartsch commented Sep 12, 2017

@brettsmason @heyflo @sveggiani I tried to accomplish the same: A grid-margin-x grid and thought the xy-grid-layout mixin should be exactly what is needed for that. So I used the following code:

.item-row{
  @include xy-grid();
    @include xy-gutters($negative: true);

    @include xy-grid-layout(1, '.item-row__cell', $gutter-type: margin);

    @include breakpoint(medium) {
        @include xy-grid-layout(2, '.item-row__cell', $gutter-type: margin);
    }
}

It wasn´t clear to me how to remove the padding gutter to achieve the margin gutter. Instead of using @include xy-gutters($negative: true); it would make more sense to me to do something like @include xy-grid($gutter-type: margin);

@heyflo
Copy link

heyflo commented Sep 12, 2017

@jmartsch I think you are right, using a parameter on the xy-grid just like like xy-grid($gutter-type: margin); which could be null by default, would actually remove the need to use another mixin to apply negative margin on that element when there is an xy-container as a parent.

Maybe they thought about more usages of that mixin (xy-gutters) that we don't think of. @kball or @brettsmason if you can shed some lights on this, I would be thanksful 👍

@Shipwreck
Copy link

I don't have anything to contribute at the moment but have run into the same issue.

Very much appreciate the solutions provided but a bit surprised this doesn't work better out of the box in regards to building semantically.

@tbredin
Copy link
Contributor

tbredin commented Feb 14, 2019

Hi there, a question (and perhaps a feature request?) — many of the designs I work with don't have square gutters. What do I mean by this? For example, gutters might be 20px wide and 18px tall. I'm not sure if the grid supports this currently?

My suggestion would be to support the following, for example:

$grid-margin-gutters: (
  small: 1.125rem 1rem,
  large: 1.25rem 1.125rem
);

Where I'm setting these in rem instead of pixels.

Is this possible / planned? I couldn't find an issue or much about this online.

Next step for me is to see if I can override some of the foundation mixins in my build and get something like this working. Will report back.

@DanielRuf
Copy link
Contributor

Thanks everyobne for the helpful feedback.

We are closing this very long issue as separate issues and questions should be reported as new single issues.

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

No branches or pull requests