-
-
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
[XGrid] Make Infinite loading support rowCount
#1715
Changes from 3 commits
ada4c4d
21196bd
ddc90de
4c0a5b1
1f94832
e520dbc
dd7af2c
b841f96
4ab7cd0
97c86a8
802ff36
9dd3579
8bbbe71
6222f1e
8fc923e
369cb68
60a7adc
11d936a
f9f2ab1
59aa187
658879f
f81a1cf
4290995
323f10a
02b02de
b481127
42f0830
c08eb4b
2007edd
cdcdfee
f96f635
828a186
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 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,6 +3,7 @@ import { | |||||||||||||
GRID_RESIZE, | ||||||||||||||
GRID_NATIVE_SCROLL, | ||||||||||||||
GRID_ROWS_SCROLL, | ||||||||||||||
GRID_VIRTUAL_PAGE_CHANGE, | ||||||||||||||
} from '../../../constants/eventsConstants'; | ||||||||||||||
import { GridApiRef } from '../../../models/api/gridApiRef'; | ||||||||||||||
import { GridVirtualizationApi } from '../../../models/api/gridVirtualizationApi'; | ||||||||||||||
|
@@ -154,6 +155,12 @@ export const useGridVirtualRows = ( | |||||||||||||
if (containerProps.isVirtualized && page !== nextPage) { | ||||||||||||||
setRenderingState({ virtualPage: nextPage }); | ||||||||||||||
logger.debug(`Changing page from ${page} to ${nextPage}`); | ||||||||||||||
|
||||||||||||||
apiRef.current.publishEvent(GRID_VIRTUAL_PAGE_CHANGE, { | ||||||||||||||
currentPage: page, | ||||||||||||||
nextPage, | ||||||||||||||
api: apiRef, | ||||||||||||||
}); | ||||||||||||||
Comment on lines
+154
to
+159
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. This seems to be an implementation detail of the virtualization. I don't think that we should expose it.
Suggested change
Instead, prefer listening to the viewport or anything that is resilient to a more standard virtualization implementation like in react-window, react-virtuoso, react-virtual. 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 can call the API here directly but that would deviate from the established pattern. I added that event because of it. We can not document the event and keep it private? 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. Could we listen to the scroll event instead? Or anything close to https://ag-grid.com/react-grid/viewport/. But avoid at ALL cost to depend on any virtualization implementation detail? Based on #1911 and #1903, I think that there is a nonnegligible chance that this "page" notion won't stick around. 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 can listen to any of the scroll events we currently have but I would still need to calculate where the last currently virtualized row is and when the new one comes in. |
||||||||||||||
requireRerender = true; | ||||||||||||||
} else { | ||||||||||||||
if (!containerProps.isVirtualized && page > 0) { | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
/** | ||
* Object passed as parameter of the virtual page change event. | ||
*/ | ||
export interface GridVirtualPageChangeParams { | ||
/** | ||
* The current page. | ||
*/ | ||
currentPage: number; | ||
/** | ||
* The next page. | ||
*/ | ||
nextPage: number; | ||
/** | ||
* Api that let you manipulate the grid. | ||
*/ | ||
api: any; | ||
} |
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.
This section seems weird. Why not use the
apiRef.current.updateRows()
API? It supports additions.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.
Maybe I didn't understand the
updateRows
but from what I can see it supports update, delete, and adding new rows but it can't replace existing rows, which is what I need here. but now thinking about it maybe it would be better to extendupdateRows
🤔. The only thing missing is the start and end indexes from which the update needs to happen.