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

[DataGrid] Blank space in the scroll container #414

Closed
2 tasks done
hinsxd opened this issue Oct 8, 2020 · 12 comments
Closed
2 tasks done

[DataGrid] Blank space in the scroll container #414

hinsxd opened this issue Oct 8, 2020 · 12 comments
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!
Milestone

Comments

@hinsxd
Copy link

hinsxd commented Oct 8, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

https://codesandbox.io/s/data-grid-bug-388qz
When scrolling to bottom, the rows "jumped" and unnecessary spacing appears at the bottom.

bug

Expected Behavior 🤔

Scrolls normally to the bottom, with the last item sticking to the bottom.

Steps to Reproduce 🕹

See codesandbox

Steps:

Context 🔦

Your Environment 🌎

Tech Version
@material-ui/data-grid" "^4.0.0-alpha.6"
React 16.13.1
Browser macOS Chrome v85
TypeScript
etc.
@hinsxd hinsxd added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 8, 2020
@hinsxd hinsxd changed the title https://codesandbox.io/s/data-grid-bug-388qz [Datagrid] Rows "Jumps" when scrolling to bottom. Oct 8, 2020
@oliviertassinari
Copy link
Member

When scrolling to bottom, the rows "jumped" and unnecessary spacing appears at the bottom.

I can observe the spacing that shouldn't be present, however, what do you mean by "jump" do you have a recording?

@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 8, 2020
@hinsxd
Copy link
Author

hinsxd commented Oct 8, 2020

Yes here it is. I sometimes encounter this but sometimes don't.
https://imgur.com/Mqfpo0a

And this should be related too. I check DevTools and noticed that the last row is not rendered sometimes.
https://imgur.com/xXqpyVF

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Oct 8, 2020
@oliviertassinari oliviertassinari changed the title [Datagrid] Rows "Jumps" when scrolling to bottom. [DataGrid] Rows "Jumps" when scrolling to bottom. Oct 8, 2020
@hinsxd
Copy link
Author

hinsxd commented Oct 15, 2020

Is there a way to disable virtualization? Many users won't load more than 100 rows and virtualization seems overkill sometimes.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 15, 2020

@hinsxd We could consider this option. However, I would recommend we wait for #193 to land before exploring it further. But in any case, we want to fix this bug.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Dec 14, 2020

I will be looking into this. Let's see what I can find. Some code paths and context to look into could help me.

@royletron
Copy link

royletron commented Feb 11, 2021

Hi @oliviertassinari I am also interested in disabling the virtualisation for small grids but with lots of UI elements. I am not entirely sure how it relates to #193 but would be eager to understand what is holding back the ability to set rowBuffer like we currently set columnBuffer?

Screenshot 2021-02-11 at 15 17 58

For reference this is what we see with the scroll event, if we have a monitor high enough to show all 100 rows (which is what we set our pagination to anyway) then the performance is fine, it's just the scrolling into the virtual rows that is causing this epic event.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 11, 2021

if we have a monitor high enough to show all 100 rows

@royletron this sounds related to #270. I was advocating that the overscan logic is suboptimal. I suspect that for large screens, we render way too many rows outside of the viewport. I was advocating that the height of the virtualization screen shouldn't be a variable in the equation, the height of the row, on the other hand, should.

Do you have a codesandbox with the display conditions so we can investigate?

@royletron
Copy link

@oliviertassinari I'll have a go and get back to you. We are using a few custom renderers on our columns which allow for interaction so it seems mounting those from the buffer is expensive.

@royletron
Copy link

@oliviertassinari check out https://codesandbox.io/s/mui-x-414-xlhsq

Particularly scrolling to the bottom of the grid.
Like I say, I understand that this is quite intensive, but our solution is to limit the number of rows shown at once, so controlling the virtualisation would be ideal.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2021

@royletron Perfect. Note that renderCell expects a render function, not a component. The hooks work by chance inside them.

We have little room to improve. The core components weren't designed to render smoothly with x100 instances x10 Hz. If it were the objective, we would have made significantly different choices. I would recommend:

  1. Developers: Rendering simpler components, not an edit all mode.
  2. Material-UI: That we improve the overscan [RFC] Row virtualization overscanning tradeoff #270.
  3. Material-UI/Developers: Testing different virtualization technics but I don't think that any will work with the currently rendered components

As for making the core component render more smoothly. You are using v4, they are getting a bit slower in v5 because we move from JSS to emotion/styled-components. Nothing major when you have a few instances in a page, but definitely noticeable when you render x100 multiple times per second. For us, the choice was a no brainer, more flexibility with the core components largely yield more adoption than more performance.

@dtassone dtassone self-assigned this Mar 25, 2021
@oliviertassinari oliviertassinari added this to the Sprint 14 milestone Mar 29, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 14, 2021

and unnecessary spacing appears at the bottom.

I believe we didn't solve this part of the issue. It can be seen it on https://master--material-ui-x.netlify.app/components/data-grid/demo/.

Screenshot 2021-04-14 at 13 59 08

@oliviertassinari oliviertassinari changed the title [DataGrid] Rows "Jumps" when scrolling to bottom. [DataGrid] Blank space in the scroll container Apr 15, 2021
@dtassone dtassone removed their assignment Apr 15, 2021
@m4theushw
Copy link
Member

The unnecessary blank space at the bottom pointed by #414 (comment) was fixed in #2673 and released in the latest beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

6 participants