carbon-design-system / ibm-products

A Carbon-powered React component library for IBM Products
https://carbon-for-ibm-products.netlify.app/
Apache License 2.0
87 stars 120 forks source link

feat(Datatable): table header #1973

Closed Ratheeshrajan closed 1 year ago

Ratheeshrajan commented 2 years ago

Contributes to #1931

WKC data table does not include Carbon for IBM Products PAL table headers.

What did you change?

How did you test and verify your work?

Storybook

netlify[bot] commented 2 years ago

Deploy Preview for carbon-for-ibm-products ready!

Name Link
Latest commit ab1b8f3c3e96b16da31ccf2d483890a881fd2f70
Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/62e0a4bccb05a800088803a8
Deploy Preview https://deploy-preview-1973--carbon-for-ibm-products.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Ratheeshrajan commented 2 years ago

@matthewgallo Thanks for spotting the issue in the left panel. I have fixed that one in the last commit and now it looks like the following screenshot. Screenshot 2022-05-13 at 3 30 40 PM

I have moved the left panel inside TableContainer to fix that issue.

matthewgallo commented 2 years ago

Hey @Ratheeshrajan, the fix for the left panel story/placement of the toolbar looks great. But my only reservation now is that there seems to be double scrollbars on some cases which is something we should fix.

This screenshot is from the Batch actions story: image

Ratheeshrajan commented 1 year ago

Hey @Ratheeshrajan, the fix for the left panel story/placement of the toolbar looks great. But my only reservation now is that there seems to be double scrollbars on some cases which is something we should fix.

This screenshot is from the Batch actions story: image

@matthewgallo fixed the double scrollbar issue. Please have a look.

elycheea commented 1 year ago

Also just noticed that the data table header now blends into the table. Might have been a conflict when you merged from main?

image

Ratheeshrajan commented 1 year ago

Also just noticed that the data table header now blends into the table. Might have been a conflict when you merged from main?

image

Sorry! I am not clear about this comment. Could please elaborate?

davidmenendez commented 1 year ago

@elycheea can you please review @Ratheeshrajan response?

elycheea commented 1 year ago

@davidmenendez We discussed via Slack. Ratheesh will add the dense header variant and will follow up with design on clarification since the styles are coming from the Carbon default.

Ratheeshrajan commented 1 year ago

@elycheea @matthewgallo For dense header, I added one data table header variant. I'm also waiting for Marion's response on the grey10 header background.

Ratheeshrajan commented 1 year ago

@elycheea updated the header background to match PAL guidelines.

matthewgallo commented 1 year ago

Discussed with both @elycheea and @Ratheeshrajan over slack but there seems to be some layout issues with the pagination appearing at the bottom of the page in the deploy preview. After that is fixed, I think we'll be ready to merge this one!

Ratheeshrajan commented 1 year ago

@matthewgallo, @elycheea I just realized that the table should only have the horizontal scroll bar according to the guideline, and I discussed this with Marion. The vertical scrollbar should work with the page. As a result, I removed all previous height settings.