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

Table with sticky header is incompatible with Table.ScrollContainer #6960

Open
1 of 2 tasks
nabby27 opened this issue Oct 10, 2024 · 8 comments
Open
1 of 2 tasks

Table with sticky header is incompatible with Table.ScrollContainer #6960

nabby27 opened this issue Oct 10, 2024 · 8 comments

Comments

@nabby27
Copy link

nabby27 commented Oct 10, 2024

Dependencies check up

  • I have verified that I use latest version of all @mantine/* packages

What version of @mantine/* packages do you have in package.json?

7.13.2

What package has an issue?

@mantine/core

What framework do you use?

Vite

In which browsers you can reproduce the issue?

Chrome

Describe the bug

I've recently been making a comparison table of features for different pricing plans. With this case and to be responsive I wanted both the header and the first column to be sticky.

For that I put the parameters stickyHeader and stickyHeaderOffset={0} to the table and wrapped the table with the Table.ScrollContainer component as indicated in the documentation. But the combination of both makes the stickyHeader stop working.

If possible, include a link to a codesandbox with a minimal reproduction

https://stackblitz.com/edit/mantine-bug-table-sticky-header-with-scrollcontainer?file=src%2FTableDemo.tsx

Possible fix

I've been investigating and it seems that the sticky header doesn't get along well with having overflows above it. Ideally, this result could be achieved: https://codepen.io/chriscoyier/pen/yLVNErX

Self-service

  • I would be willing to implement a fix for this issue
@nabby27
Copy link
Author

nabby27 commented Oct 10, 2024

I have created a $100 bounty because I am very interested in seeing this solved.

@RohanMishra315
Copy link

RohanMishra315 commented Oct 10, 2024

@nabby27
Copy link
Author

nabby27 commented Oct 14, 2024

Hi @RohanMishra315, Thanks for the stackblitz, I'm sure it can be helpful to the Mantine team. I want this to be integrated into Mantine and have the team support it. Also, the horizontal scrolling in your example is for the entire page, not just the table content.

@nicojav
Copy link

nicojav commented Oct 14, 2024

@nabby27 added a fix with a demo in the PR

@nabby27
Copy link
Author

nabby27 commented Oct 15, 2024

Hi @nicojav! Thanks for your PR and the stackblitz to see how it works, it's very helpful.

Personally I think the solution doesn't seem the most appropriate, it seems more like a patch than a solution in itself. Why does the scroll container have to have a height of 95vh and not 90vh or 99vh? It seems random.

I would expect the solution to be in the table and not in the scroll container as this can be used in cases where that 95vh breaks with what is expected.

Anyway, I will wait for the Mantine team to decide if the solution is valid or not.

PS: I'm speaking without having worked on the Mantine codebase so I could be wrong.

@nicojav
Copy link

nicojav commented Oct 21, 2024

Hi @nabby27 i understand, was trying to applying a quickfix using only CSS.
Here its an updated solution using another approach #7004 with a working demo too if you want to check it out.

@rtivital
Copy link
Member

@nabby27 Your "bounty" just generates spam PRs that cannot be merged. Please remove it to save me time for maintenance. I'm not ready to spend time reviewing PRs from someone who does not use the library and does not understand the code, considering that none of the above fixes the issue reasonably.

@nabby27
Copy link
Author

nabby27 commented Oct 21, 2024

Hi @rtivital, sure, if you think it doesn't help, I'll remove the bounty. I appreciate your understanding. I just wanted to make things easier and save you time. I see that in this case, it's not like that. Sorry for the inconvenience.

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 a pull request may close this issue.

4 participants