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

[DataGridPro] Rework onRowsScrollEnd to use IntersectionObserver #8672

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Apr 19, 2023

Fixes #4371
Fixes #9022

Preview: https://deploy-preview-8672--material-ui-x.netlify.app/x/react-data-grid/row-updates/#infinite-loading

TODO:

@DanailH DanailH added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! plan: Pro Impact at least one Pro user labels Apr 19, 2023
@DanailH DanailH self-assigned this Apr 19, 2023
@DanailH DanailH changed the title Rework 'onRowsScrollEnd' to use 'IntersectionObserver' [DataGridPro] Rework 'onRowsScrollEnd' to use 'IntersectionObserver' Apr 19, 2023
@DanailH DanailH changed the title [DataGridPro] Rework 'onRowsScrollEnd' to use 'IntersectionObserver' [DataGridPro] Rework onRowsScrollEnd to use IntersectionObserver Apr 19, 2023
@mui-bot
Copy link

mui-bot commented Apr 19, 2023

@DanailH DanailH requested a review from romgrk July 3, 2023 14:21
* Stores the ref of the element at the bottom of the virtual scroller content that triggers the infinite loading.
* @ignore - do not document.
*/
unstable_infiniteLoadingTriggerRef: (node: HTMLElement | null) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in private API?

Copy link
Member

Choose a reason for hiding this comment

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

I've replaced it with a getInfiniteLoadingTriggerElement method in the private API and moved the interface to the MIT package for the sake of type safety (getInfiniteLoadingTriggerElement is being called in the MIT package's useGridVirtualScroller)

Comment on lines 45 to 51
const InfiniteLoadingTriggerElement = styled('div')({
position: 'sticky',
left: 0,
width: 0,
height: 0,
});

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love how the infinite loading logic is spilling in this file. Is there any other way we can separate the logic?

Instead of exposing unstable_infiniteLoadingTriggerRef on the API, maybe we could expose (a) function(s) to add the trigger element?

Copy link
Contributor

Choose a reason for hiding this comment

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

The function could take as input the props, so the props.rowsLoadingMode === 'client' (below) would not need the (rootProps as any) cast that's required here.

Copy link
Member

Choose a reason for hiding this comment

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

Moved the trigger to the useGridInfiniteLoader hook 👍🏻

@cherniavskii cherniavskii requested a review from romgrk March 2, 2024 12:48
Comment on lines 75 to 77
if (triggerElement.current) {
observer.current?.unobserve(triggerElement.current);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it applies but there is also observer.current?.disconnect(), which can help avoid storing the observed element ref sometimes.

Copy link
Member

Choose a reason for hiding this comment

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

Good point! I used disconnect instead of unobserve.

I still need to keep track of the trigger element to observe it after recreating IntersectionObserver (e.g. when marginBottom changes): https://github.com/mui/mui-x/pull/8672/files#diff-626d69f7d7c9aebfab3fdc6388cd0f88a38b6d9e51c567e76f63a25d63895b10R81-R83

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! feature: Row loading Related to the data grid Row loading features plan: Pro Impact at least one Pro user v7.x
Projects
Status: Done
6 participants