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

[Bug]: DataTable size top/bottom padding differences #13213

Open
2 tasks done
jsehull opened this issue Feb 22, 2023 · 5 comments
Open
2 tasks done

[Bug]: DataTable size top/bottom padding differences #13213

jsehull opened this issue Feb 22, 2023 · 5 comments

Comments

@jsehull
Copy link
Contributor

jsehull commented Feb 22, 2023

Package

carbon-components

Browser

Chrome

Package version

v1.21.0, also found in most recent Storybook v1.23.1

React version

v17.0.2

Description

Hey team, I ran into some oddities with some cell spacing and wanted to find out if this is intended or not. If I can receive some guidance here I would be glad to work on a fix.

Any workaround is likely just adding custom padding to the table.

It's possible that the design is accomplished with these values and it's not considered a problem. Maybe this IS intentional?

Expected: When using DataTable size ranges (i.e. xs, sm, xl, etc), cell padding should INCREASE when using greater size levels.

Experience: Padding for different size levels are inconsistent and, in some cases, missing.


Three bug questions:

  1. Is it expected for LG to have ZERO top AND bottom padding?
  2. Is it purposeful for XL to have ZERO bottom padding?
  3. Should SM and MD have different padding values for top AND bottom? Or should they be matching values, as is the case with XS?

Notes on top/bottom padding for the following sizes:

XS - equal values for both
SM, MD - different padding values
Medium example:
Balancer

LG - ZERO top/bottom padding
Balancer

XL size has no bottom padding
image

Reproduction/example

https://react.carbondesignsystem.com/?path=/story/components-datatable-basic--playground&args=size:md

Steps to reproduce

  1. Go to Storybook > DataTable > Default > Playground (or any playground that has size testing available)
  2. Open Dev Tools to see padding differences between individual Row cells
  3. Swap between different size values and compare

This is happening in a side project for me, but I'm finding it's also present in the most recent Storybook, so that's where I chose to link it for quick testing. Please let me know if I should recreate in a code sandbox.

Suggested Severity

Severity 4 = Unrelated to a user task, has a workaround or does not need a workaround.

Application/PAL

n/a

Code of Conduct

@andreancardona
Copy link
Contributor

@laurenmrice Hey Lauren would be curious to get your thoughts on if this is a bug or not, thank you!

@laurenmrice
Copy link
Member

Hi @jsehull 👋

Is this padding issue something you are noticing at smaller screen sizes? By default, the text within a data table row should be vertically centered inside of the LG, MD, SM, and XS row height sizes. The top and bottom padding will always be the same. However, the XL size has a 15px top padding and 32px bottom padding.

data table row paddings copy 2

At smaller screen sizes, our design intent is that the text within the data table rows should scroll horizontally but maintain the same vertical heights listed above^. However in the current code implementation, I can see that some text wraps and makes the row height grow arbitrarily in size, and the horizontal scroll is limited. We have another issue here around this topic that is currently open, and we are working on resolving it with the help of teams on IBM Cloud. There also were some accessibility concerns around a table vertically and horizontally scrolling simultaneously, which we need to consider. Overall if you are seeing padding inconsistencies like this at smaller screens, we are currently investigating a solution for it.

@jsehull
Copy link
Contributor Author

jsehull commented Feb 28, 2023

Thanks for the insights @laurenmrice.

My initial findings were for desktop. Testing now on small devices, I get the same results posted above. For quick testing, visiting the DataTable Storybook helps illustrate the situation (Dev Tools needed).

Based on the designs, it seems like the padding is not doing a few things:

  1. does NOT use matching top and bottom values
  2. does NOT have unique values per size (i.e. small and medium are the same)
  3. XL does NOT have bottom padding currently

Maybe something updated and the values no longer match the designs? Am I missing something?

@tay1orjones
Copy link
Member

Yeah, it might be worth another pass here at these values to see if we can get them to match the spec. I think currently the situation is the values are derived from the constraints of row/cell sizing in tables, which can be finicky.

For instance, some things I noticed playing around just now:

  • Row heights are specified on rows, independently of the cell padding, meaning the cell padding is not solely determining the height of the row.
  • The padding values may be tied to the line height not being specifically set. If I modify the padding of md to be 11px, the row height is no longer to spec and measures 41px.

I'm all for exploring this to match the spec. As part of the exploration, we need to be careful to not introduce regressions for consumers relying on this padding. One way we can try to avoid regression would be to visually snapshot (vrt) all table row sizes with Percy to ensure there are no regressions in height across different table variants.

@tay1orjones tay1orjones added role: dev 🤖 severity: 3 https://ibm.biz/carbon-severity labels Mar 6, 2023
@tay1orjones
Copy link
Member

Also, as far as a workaround for this - it's possible to override these values as-is with your own styles. If anyone has done this already to get the table to match the spec exactly, it would be a great place to start this exploration after the VRT snapshots are added.

@tay1orjones tay1orjones moved this from 🪆 Needs Refined to ⏱ Backlog in Design System Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ⏱ Backlog
Development

No branches or pull requests

5 participants