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.

Sign up for GitHub

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

Jump to bottom

fix(condensed-grid): moved vertical spacing to col #4577

Closed
wants to merge 3 commits into from
Closed

fix(condensed-grid): moved vertical spacing to col #4577

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5b7046
fix(condensed-grid): moved vertical spacing to col
photodow Nov 5, 2019
f014da0
Merge branch 'master' into col-bot-marg
joshblack Nov 15, 2019
8a0c0a1
Merge branch 'master' into col-bot-marg
joshblack Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions packages/grid/scss/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,12 +320,9 @@ $carbon--aspect-ratios: ((16, 9), (2, 1), (4, 3), (1, 1), (1, 2));
@include carbon--make-row();
}

.#{$prefix}--grid--condensed .#{$prefix}--row:not(:last-of-type) {
margin-bottom: $condensed-gutter;
}

.#{$prefix}--row--condensed + .#{$prefix}--row--condensed {
margin-top: $condensed-gutter;
.#{$prefix}--grid--condensed [class*='#{$prefix}--col-'] {
photodow marked this conversation as resolved.
Show resolved Hide resolved
padding-top: $condensed-gutter / 2;
padding-bottom: $condensed-gutter / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the row doesn't wrap, won't the padding bottom be half the size now?

Also will the first row effectively have a padding-top now that the first set of columns has a padding top?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is what I ended up running into as well when trying to implement earlier 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the row doesn't wrap, won't the padding bottom be half the size now?

@vpicone half the size of what? Going this route the top row will have 1px on top, and the last row (whether it wraps or not) will have 1px on the bottom. It evenly distributes it minimizing/equalizing any lopsidedness. Part of my thinking around this is that the left and right of the grid is offset 1px already due to the same method being used on horizontal spacing, so this would put an even 1px transparent border around the whole grid whether it wraps, or not, or whether it has one column or hundreds.

  1. Another way to look at it is do padding/margin bottom 2px, and top 0. This looks more inlined with what y'all were doing with the rows, although it looks like y'all were trying to remove the spacing on the last row (for good reason). This method would have 2px on the bottom whether it wraps or not.
.#{$prefix}--grid--condensed [class*='#{$prefix}--col-'] {
    padding-bottom: $condensed-gutter;
}
  1. A third option could to add margin top 2px, and margin bottom 2px. The margins should collapse and distribute the spacing evenly throughout the board, but now you have 2px on top and bottom of the row/grid whether it wraps or not.
.#{$prefix}--grid--condensed [class*='#{$prefix}--col-'] {
    margin-top: $condensed-gutter;
    margin-bottom: $condensed-gutter;
}

I thought maybe we could do some cool stuff with the nth-child, but that might get a little whacky when users start mixing column sizes in a grid layout.

I'm open to other possible solutions. @joshblack I figured you were dealing with some of the same questions when ya'll first implement it. Just trying to open up the discussion here, and see if we can find some sort of solution that works everyone. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

We're going from 0px top to 1px and from 2px bottom to 1px right? Just making sure that was the intent Would like to see some more examples with wraps/no-wraps before and after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the codesandbox I built in #4555 might help you see more examples, or maybe you're asking for more examples on top of that?

https://codesandbox.io/s/unruffled-raman-6rl7u?fontsize=14

Right now, the columns have no vertical spacing at all. The vertical spacing is put on bx--row. So wrapping they won't have any vertical spacing. bx--row will also remove the spacing if it's the last/only row in the condensed grid.

}

@include carbon--make-grid-columns($breakpoints, $grid-gutter);
Expand Down