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] Better fix the scrollToIndexes logic #1969

Merged
merged 3 commits into from
Jun 29, 2021

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jun 26, 2021

This is a follow-up #1949 (comment), I'm pushing the quality of the solution one step further, by doing a plain copy & paste of the Autocomplete's logic.

The previous behavior: https://codesandbox.io/s/material-demo-forked-j7i1x?file=/package.json.

before.mp4

The new behavior: https://codesandbox.io/s/material-demo-forked-nzkwd?file=/package.json

new.mp4

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Jun 26, 2021
@dtassone
Copy link
Member

it doesn't really work on https://deploy-preview-1969--material-ui-x.netlify.app/components/data-grid/
Try keeping the arrow down
then up,
then go down with the scroll thumb quite a bit
and repeat the keyboard arrows test

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 28, 2021

@dtassone True, now, what's the root cause? Is it the issue from the changes or from the rendering logic? AFAIK, it's surfacing that the rendering logic is not correct, with a now correct virtual scroll logic, so I would argue it's a step forward (enforced by the test case), even if it makes the issue with the virtualization more visible.

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

It's not a perfect solution but it's better than what we have. The new logic still doesn't fix #1818 but I can improve it in #1871. In my tests, when I call scrollToindexes with a rowIndex much lower than the one that is currently displayed it doesn't scroll.

@oliviertassinari oliviertassinari merged commit 2b0daf2 into mui:master Jun 29, 2021
@oliviertassinari oliviertassinari deleted the grid-fix-scroll-logic branch June 29, 2021 14:17
@oliviertassinari
Copy link
Member Author

@m4theushw Yes, one correct test case at a time :). Now, I think that we get the fix of #1818 for free by solving #1911.

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

Successfully merging this pull request may close these issues.

None yet

3 participants