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

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

Closed
wants to merge 3 commits into from

Conversation

photodow
Copy link
Contributor

@photodow photodow commented Nov 5, 2019

Closes #4555

Changelog

Changed

  • moved vertical spacing to columns

@photodow photodow requested a review from a team as a code owner November 5, 2019 22:21
@ghost ghost requested review from asudoh and vpicone November 5, 2019 22:21
@asudoh asudoh requested a review from a team November 5, 2019 22:32
@ghost ghost requested review from jillianhowarth and removed request for a team November 5, 2019 22:32
@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for the-carbon-components ready!

Built with commit 8a0c0a1

https://deploy-preview-4577--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for carbon-elements ready!

Built with commit 8a0c0a1

https://deploy-preview-4577--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for carbon-components-react ready!

Built with commit 8a0c0a1

https://deploy-preview-4577--carbon-components-react.netlify.com

margin-top: $condensed-gutter;
.#{$prefix}--grid--condensed [class*='#{$prefix}--col-'] {
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.

Copy link
Contributor

@vpicone vpicone left a comment

Choose a reason for hiding this comment

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

Couple Qs

@joshblack joshblack requested a review from a team as a code owner November 19, 2019 23:17
@ghost ghost requested a review from joshblack November 19, 2019 23:17
@joshblack
Copy link
Contributor

Any other questions @vpicone? Any chance to review @asudoh?

@asudoh
Copy link
Contributor

asudoh commented Nov 19, 2019

The code is fine to me, waiting for visual review to ensure that the UI is good with the new code.

@joshblack
Copy link
Contributor

joshblack commented Dec 4, 2019

Hey @photodow! 👋 Going through slate PRs and closing them. Feel free to comment and we can 100% re-open it if you have a sec to respond to some of the comments above!

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

Successfully merging this pull request may close these issues.

[condensed grid] wrapping columns collapse
4 participants