search

KevinVandy / material-react-table

A fully featured Material UI V5 implementation of TanStack React Table V8, written from the ground up in TypeScript
https://material-react-table.com
MIT License
1.46k stars 418 forks source link

Rows disappear exiting full screen #311

Closed duckboy81 closed 1 year ago

duckboy81 commented 1 year ago

material-react-table version

v1.5.6

react & react-dom versions

v5.11.4 & v18.0.10

Describe the bug and the steps to reproduce it

Troubleshooting this right now. It appears the bad combo is enableRowVirtualization and enableBottomToolbar. Disabling either one of these will prevent the issue.

Digging into it, MRT_BottomToolbar seems to be the culprit. If you pass your own renderBottomToolbar, MRT_BottomToolbar is not used and there is no issue.

Removing the following ref prop from the Toolbar component in MRT_BottomToolbar seems to prevent the issue, but I'm doing some more research now.

https://github.com/KevinVandy/material-react-table/blob/aa72893599d091fd0a77f7d1732219f36e13e33d/packages/material-react-table/src/toolbar/MRT_BottomToolbar.tsx#L45-L51

Interestingly, manually attaching my own MutationObserver to the "table body section" (I've since forgot what it's actually called) would show that the "table body section" is removed from DOM and the observer loses any new updates when the bug is occurring. When things work normally, the MutationObserver is unaffected and new updates are passed correctly.

The final update the MutationObserver sees when exiting fullscreen is a 0px "table body section" height so all rows are removed except for the number of rows specified in the overscan setting.

259

Minimal, Reproducible Example - (Optional, but Recommended)

https://codesandbox.io/s/gracious-jepsen-ndef3b?file=/src/TS.tsx

Screenshots or Videos (Optional)

No response

Do you intend to try to help solve this bug with your own PR?

Maybe, I'll investigate and start debugging

Terms

KevinVandy commented 1 year ago

Nice investigation, was going to say that it sounds like a duplicate of #259 , but yeah, this still seems to be an issue. The toolbar ref being a culprit would be really weird, but possible I guess. I thought it was an issue with @tanstack/react-virutal itself, but probably has to do with the table deleting itself when going fullscreen. That logic is in the MRT_TableRoot component.

duckboy81 commented 1 year ago

It's interesting for sure. I'm chasing a lead in the following code. It seems if I set bottomToolbarHeight to something small (0 or even 1) it had no issues.

https://github.com/KevinVandy/material-react-table/blob/aa72893599d091fd0a77f7d1732219f36e13e33d/packages/material-react-table/src/table/MRT_TableContainer.tsx#L28-L40

The only consumer of totalToolbarHeight is the TableContainer component. So I'm starting to look into this as well.

https://github.com/KevinVandy/material-react-table/blob/aa72893599d091fd0a77f7d1732219f36e13e33d/packages/material-react-table/src/table/MRT_TableContainer.tsx#L54-L69

We'll see what I come up with.

KevinVandy commented 1 year ago

@duckboy81 Welp, you have found the ugliest code in the MRT codebase probably lol. I actually wish to delete it, but I think it was the only way to do... something... I think get rid of a scrollbar in fullscreen mode?

duckboy81 commented 1 year ago

@KevinVandy Alright, pull request submitted. If that comes through clean on your end, I wonder if we should also update MRT_TopToolbar and/or MRT_TablePaper to match.

KevinVandy commented 1 year ago

@KevinVandy Alright, pull request submitted. If that comes through clean on your end, I wonder if we should also update MRT_TopToolbar and/or MRT_TablePaper to match.

Adding the if checks? probably