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(dataGrid): header scroll issues #6135

Merged

Conversation

devadula-nandan
Copy link
Contributor

@devadula-nandan devadula-nandan commented Sep 30, 2024

Closes #5948

  • The width is now dynamically set on window resize. Previously, useIsomorphicEffect used to set the width only during the initial load. the effect didn't used to run on gridRef changes, now we are getting the width from it and passing it as dependency.
  • Added a scroll event to the header to sync with the body. Previously, only scrolling the body would sync the header, but now scrolling the header also syncs the body.
  • Fixed the last column using the flex: 1 0 auto property.
Screen.Recording.2024-10-01.at.12.37.50.PM.mov

What did you change?

packages/ibm-products-styles/src/components/Datagrid/styles/_datagrid.scss
packages/ibm-products/src/components/Datagrid/Datagrid/DatagridVirtualBody.tsx
packages/ibm-products/src/components/Datagrid/Datagrid/DatagridRow.tsx

How did you test and verify your work?

storybook, dev tools touch simulation.

Copy link

netlify bot commented Sep 30, 2024

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

Name Link
🔨 Latest commit 802887d
🔍 Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/66fb9105da6f350008c67799
😎 Deploy Preview https://deploy-preview-6135--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 configuration.

@devadula-nandan devadula-nandan changed the title fix(Datagrid): header scroll issues fix(dataGrid): header scroll issues Sep 30, 2024
@devadula-nandan devadula-nandan marked this pull request as ready for review October 1, 2024 06:57
@devadula-nandan devadula-nandan requested a review from a team as a code owner October 1, 2024 06:57
@devadula-nandan devadula-nandan requested review from matthewgallo and AlexanderMelox and removed request for a team October 1, 2024 06:57
Copy link
Contributor

@amal-k-joy amal-k-joy left a comment

Choose a reason for hiding this comment

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

Good work

Copy link
Contributor

@AlexanderMelox AlexanderMelox left a comment

Choose a reason for hiding this comment

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

LGTM!

@AlexanderMelox AlexanderMelox added this pull request to the merge queue Oct 4, 2024
Merged via the queue into carbon-design-system:main with commit 9d60c0c Oct 4, 2024
24 checks passed
@wkeese
Copy link
Contributor

wkeese commented Nov 3, 2024

Thanks @devadula-nandan, I confirm it works on https://carbon-for-ibm-products.netlify.app/?path=/story/ibm-products-components-datagrid--infinite-scroll&globals=viewport:basic and https://carbon-for-ibm-products.netlify.app/?path=/story/ibm-products-components-datagrid--with-virtualized-data&globals=viewport:basic.

Do you have an opinion about @carbon/react's scrolling strategy, which scrolls on the table wrapper via the stickyHeader... vs. @carbon/ibm-products strategy of scrolling on thead/tbody?

@devadula-nandan
Copy link
Contributor Author

Hi @wkeese, here is my playground while i was experimenting on sticky header with horizontal and vertical scroll, on a constrained table. its not a carbon example but
the div that contains table element needs to have overflow auto and the thead to have sticky and top-0, which i consider an ideal implementation
Screenshot 2024-11-26 at 6 57 01 PM

however carbon's implementation does have sticky and top 0 on thead, but columns doesnt have defined widths like tanstack does, hence we cant get horizontal scrolling to work effectively afaik
Screenshot 2024-11-26 at 7 05 25 PM

and coming to datagrid there is this dependency that got built up on certain implementations within datagrid, to an extent that we need to re architect and could also potentially bring regressions, hence we moved to tanstack-carbon way, in which u can use carbon's datatable features, and u can also bring in your own custom changes.
but i do agree overflow on tbody and thead and syncing them is an overkill

hope this helps

@wkeese
Copy link
Contributor

wkeese commented Dec 9, 2024

Thanks @devadula-nandan, that all makes sense, although it will personally take me a while to get used to a vertical scrollbar that extends up to the top of the headers.

I wonder why ibm-products Datagrid decided to break ranks with @carbon's DataTable and put separate scrolling on the table body.

But iIn any case, I agree that scrolling on the table itself (or a wrapper around the table) makes sense. Your playground seems to work perfectly.

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

Successfully merging this pull request may close these issues.

Datagrid: can't scroll headers
4 participants