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] Fix scroll error #15521

Merged
merged 3 commits into from
Nov 28, 2024
Merged

Conversation

cherniavskii
Copy link
Member

Fixes #14987 (comment)

I can't reproduce the issue, but early return makes sense here anyway.

@cherniavskii cherniavskii added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! needs cherry-pick The PR should be cherry-picked to master after merge labels Nov 20, 2024
@cherniavskii cherniavskii requested a review from a team November 20, 2024 22:05
@mui-bot
Copy link

mui-bot commented Nov 20, 2024

Deploy preview: https://deploy-preview-15521--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 805c47c

@@ -216,9 +216,14 @@ export const useGridVirtualScroller = () => {
);

const triggerUpdateRenderContext = useEventCallback(() => {
const scroller = scrollerRef.current;
if (!scroller) {
Copy link
Member

@oliviertassinari oliviertassinari Nov 20, 2024

Choose a reason for hiding this comment

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

I still can't help myself asking if this is not hiding something.

It could be about useTimeout that uses useOnMount, that uses useEffect so call the clear function async, so after the DOM node was unmounted, ref was set to null. Changing useTimeout to use useLayoutEffect to run clean-up in sync mode could solve this.

Copy link
Member Author

Choose a reason for hiding this comment

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

after the DOM node was unmounted, ref was set to null

I tried reproducing it #14987 (comment), but I couldn't get any scrolling callbacks called after the grid was unmounted.

Comment on lines -220 to +226
top: scrollerRef.current!.scrollTop,
left: scrollerRef.current!.scrollLeft,
top: scroller.scrollTop,
left: scroller.scrollLeft,
Copy link
Contributor

Choose a reason for hiding this comment

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

The ! type annotation causing yet another bug.

@cherniavskii cherniavskii merged commit ca7dd3d into mui:master Nov 28, 2024
18 checks passed
@cherniavskii cherniavskii deleted the fix-scroll-top-error branch November 28, 2024 14:51
Copy link

Cherry-pick PRs will be created targeting branches: v7.x

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! needs cherry-pick The PR should be cherry-picked to master after merge v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught TypeError: Cannot set properties of null (setting 'scrollTop')
4 participants