-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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] Add onRowsScrollEnd to support infinite loading #1199
Changes from 9 commits
c3c97d9
848ba04
ef052c4
afd4715
0fa88ca
d1d177c
8e1349b
82927cb
68ea707
e7f0482
b25717e
2052d3c
1d02608
2fe73eb
0395181
a3635c0
d5d94b4
2a0dd74
8004dea
02c22c2
1c5991d
ae1dc1c
22e0167
657fbd8
246eece
24de8a6
3bd15dd
3eac46f
0a1b799
182456d
f228c90
18c24d6
8952a1b
1d655e8
9af4f66
cf50ca5
84de4e3
6ba992e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from './useGridInfiniteLoader'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import * as React from 'react'; | ||
import { optionsSelector } from '../../utils/optionsSelector'; | ||
import { GridApiRef } from '../../../models/api/gridApiRef'; | ||
import { useGridSelector } from '../core/useGridSelector'; | ||
import { useNativeEventListener } from '../../root/useNativeEventListener'; | ||
import { GRID_SCROLL } from '../../../constants/eventsConstants'; | ||
import { gridContainerSizesSelector } from '../../utils/sizesSelector'; | ||
|
||
export const useGridInfiniteLoader = ( | ||
windowRef: React.MutableRefObject<HTMLDivElement | null>, | ||
apiRef: GridApiRef, | ||
): void => { | ||
const options = useGridSelector(apiRef, optionsSelector); | ||
const containerSizes = useGridSelector(apiRef, gridContainerSizesSelector); | ||
oliviertassinari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const isInScrollBottomArea = React.useRef<boolean>(false); | ||
|
||
const handleGridScroll = React.useCallback( | ||
oliviertassinari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(event) => { | ||
const scrollPositionBottom = | ||
event.target.scrollTop + containerSizes?.windowSizes.height + options.scrollBottomThreshold; | ||
|
||
if (containerSizes && scrollPositionBottom >= containerSizes.dataContainerSizes.height) { | ||
if (!isInScrollBottomArea.current && options.onScrollBottom) { | ||
isInScrollBottomArea.current = true; | ||
options.onScrollBottom(); | ||
DanailH marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} else { | ||
DanailH marked this conversation as resolved.
Show resolved
Hide resolved
|
||
isInScrollBottomArea.current = false; | ||
} | ||
}, | ||
[options, containerSizes], | ||
); | ||
|
||
useNativeEventListener(apiRef, windowRef, GRID_SCROLL, handleGridScroll, { passive: true }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is already a scroll event in the grid, why not use it?
This is also a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get it. Why should we care about the "scrolling" state (a boolean?)? Isn't the "scroll position" state that we are interested into? Maybe there is a subtlety between the two that I don't get. I thought that "scrolling" state was used to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I can explain. So, yes @dtassone there is a scrolling event on the grid that returns the top and left positions of the scroll. The problem is that when the virtualization kicks in those values are being reset back to 0 so it isn't possible to know the actual scroll top for the entire grid viewport. That's why I'm using this one. it works the same way and it is not fired any more times than the other but it returns the correct position. Regarding the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the issue, that's because it's the position of the rendering zone. Maybe we should modify it to add the window scroll position so you could hook into this one for the case we are covering in this PR and then we already have the base for the case of infinite scroll server side hooked into virtualisation...
It's just an approach that use the state instead of the event... Probably better perf with event on this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the main reason for not attaching the event to window is to decouple the rendering from the feature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same PR is fine as it's a very close concern There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need the change to ship value to the developers with this new callback. I would personally encourage doing it afterward IF the diff is significant (e.g. +/-100 LOCs would strongly signal that it's too large for a side improvement). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, @dtassone how about I add a third prop here https://github.com/mui-org/material-ui-x/blob/master/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualRows.ts#L145 that is PS: if that is what you meant in the first place then my appologies, I was thinking in the direction of reworking the scroll logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { GridState } from '../features/core/gridState'; | ||
|
||
export const gridContainerSizesSelector = (state: GridState) => state.containerSizes; | ||
export const gridViewportSizesSelector = (state: GridState) => state.viewportSizes; | ||
export const gridScrollBarSizeSelector = (state: GridState) => state.scrollBar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about loader
useGridInfiniteLoader > useGridInfiniteScroll?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is tricky with the name. Both seem to mean similar things. I chose the loader as that was how it was referred to in the requested feature.