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

Carbon Grid class bx--col doesn't include top or bottom padding #3860

Closed
1 task
mattalco opened this issue Aug 29, 2019 · 5 comments · Fixed by #5449
Closed
1 task

Carbon Grid class bx--col doesn't include top or bottom padding #3860

mattalco opened this issue Aug 29, 2019 · 5 comments · Fixed by #5449

Comments

@mattalco
Copy link

mattalco commented Aug 29, 2019

What package(s) are you using?

  • [ x] carbon-components
  • carbon-components-react

Detailed description

Describe in detail the issue you're having.

Our team wraps the Carbon Grid classes into a component and I've noticed what I think is an inconsistency between the code and the documentation.

The documentation (links and screenshots below) states that there's a standard 16px padding on Items (that's how I read it, which may be incorrect). When I look at the bx--col class, I only see padding on the left and right.

Is this issue related to a specific component?

This issue is related to the Grid classes. Specifically bx--col.

What did you expect to happen? What happened instead? What would you like to
see changed?

I expected a standard padding to be applied on all sides of elements tagged with the bx--col class instead of just the left and right.

What browser are you working in?

Chrome

What version of the Carbon Design System are you using?

10.5.1

What offering/product do you work on? Any pressing ship or release dates we
should be aware of?

I work within IBM on the IBM Marketplace.

Steps to reproduce the issue

  1. Create a simple Grid and with a Row and Columns. I have a link below.

  2. https://codesandbox.io/embed/carbon-components-getting-started-u9dmm

Additional information

Screen Shot 2019-08-29 at 11 32 22 AM

Screen Shot 2019-08-29 at 9 49 22 AM

@mattalco mattalco changed the title Carbon Grid class bx--col doesn't include top padding Carbon Grid class bx--col doesn't include top or bottom padding Aug 29, 2019
@joshblack
Copy link
Contributor

cc @jeanservaas would we want the grid code to ship with vertical padding? Right now we only have padding specified for gutters versus adding 16px padding to descendants in columns

@jeanservaas
Copy link
Collaborator

jeanservaas commented Aug 30, 2019

Weird, you kind of read my mind, I was noticing this discrepancy yesterday. Ideally, I think the answer to your question is yes... the grid code should ship with vertical padding that matches the side padding...

Chiu's grid guidance examples with padding had both the vertical and the left and the right.

image


Whereas the examples I've been creating for grid code tab follow what you've done for the grid demo and only have the left and the right shown.

image

But, I do want to make the point, that if people are using the 32px, the 16px or the 2px scenarios on say a dashboard situation, they want to also keep their vertical padding consistent with that choice.

BUT:
My question is, if we attach it to the grid package... is that going to be a problem for Mixed use scenarios? I'm trying to think of a case where i might be an issue -- let's talk in person about it!

@mattalco
Copy link
Author

mattalco commented Sep 4, 2019

@joshblack I just wanted to followup here given the work being done over at #3892 to see if a consensus has been reached.

@joshblack
Copy link
Contributor

@mattalco I definitely think we'll look into adding vertical padding helpers for this use-case, I don't think we'll want columns to define these by default, however.

@mattalco
Copy link
Author

mattalco commented Sep 5, 2019

@joshblack Thanks! We're super excited about the Carbon Grid component and can't wait to use it.

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

Successfully merging a pull request may close this issue.

4 participants