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

feat: allow grouped table #792

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

maloguertin
Copy link

Hi this is probably more of a WIP but I wanted to get your pulse on that. We needed to have sticky group headers in our table and I saw that not much was missing from the table implementation for it to work.

I added an example (group-table.tx)

@petyosi
Copy link
Owner

petyosi commented Nov 17, 2022

This looks about right, but I have not run it locally. Small note - can you use yarn (v1), so that you don't commit the package-lock.json?

@maloguertin
Copy link
Author

Alright I will try to get around to that today! Would you prefer to extract it to a separate component like GroupedVirtuoso?

@petyosi
Copy link
Owner

petyosi commented Nov 17, 2022

Good question - I guess yes, since the two modes are not fully compatible.

@maloguertin
Copy link
Author

Good question - I guess yes, since the two modes are not fully compatible.

Could you give me more infos on what is not compatible?

@petyosi
Copy link
Owner

petyosi commented Nov 21, 2022

The flat component accepts total count and data. The grouped one accepts groupCounts (grouped data mode is to be done, eventually). Check this: https://github.com/petyosi/react-virtuoso/blob/master/src/components.tsx#L257-L273

@maloguertin
Copy link
Author

Thanks!

@maloguertin
Copy link
Author

Does this look good to you?

@petyosi
Copy link
Owner

petyosi commented Nov 29, 2022

Sorry, I will have to get back to that over the weekend. Quite a lot going on.

@maloguertin
Copy link
Author

No worries, I noticed my implementation makes it so that the last item may be cutoff. It seems like I would need to add height to the viewport when you have a stickied group. Do you have an idea on the best way to implement this.

@petyosi
Copy link
Owner

petyosi commented Dec 1, 2022

This might be a matter of reporting additional height into the internal system. fixedHeaderHeight, I believe, but it might be tricky since that's already in use from the table header.

@maloguertin
Copy link
Author

I was wrong actually in my actual project I had a missing forward ref on a HeaderComponent, the problem is actually the inverse, there is too much height on the bottom, I'm guessing it's because the stickied group is not substracted from the totalHeight.

@maloguertin maloguertin reopened this Jan 10, 2023
@maloguertin
Copy link
Author

I fixed the conflicts and followed the new file structures for the interfaces

@adirzoari
Copy link

@maloguertin @petyosi can you add option to TableVirtuoso to have context props too? I want to pass TableBody any state variable but I don't have an option without context

@breitman
Copy link

Hey all, I am currently using GroupedVirtuoso and I need a fixed header atop the actual grouped table, and this seems like it would do the trick. Do we have an idea when this might be merged and released?

@petyosi
Copy link
Owner

petyosi commented Mar 17, 2023

I think there was something unresolved in the PR?

the problem is actually the inverse, there is too much height on the bottom, I'm guessing it's because the stickied group is not substracted from the totalHeight.

@maloguertin
Copy link
Author

I haven't had time to look into this for a while. I have managed to mostly make it work by setting a negative margin to compensate for the height inequality but it's more of a hacky solution.

@khmelevskii
Copy link

What the status of this PR? Is it too outdated and will be difficult to merge? What need to fix to be able to merge it and support grouped table.
Maybe I will try to find time for this soon

@sriranjanivenkatesan
Copy link

hello @maloguertin , Will this feature be made available anytime soon:) ?

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.

None yet

6 participants