-
Notifications
You must be signed in to change notification settings - Fork 0
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
Clean up pagination handling in Unified Data Table #4
Clean up pagination handling in Unified Data Table #4
Conversation
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.
Adding some notes related to the bug that was found.
setCurrentPageIndex(pageIndex); | ||
onUpdatePageIndex?.(pageIndex); |
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.
setCurrentPageIndex(pageIndex); | |
onUpdatePageIndex?.(pageIndex); | |
const calculatedPageIndex = pageIndex > pageCount - 1 ? 0 : pageIndex | |
setCurrentPageIndex(calculatedPageIndex); | |
onUpdatePageIndex?.(calculatedPageIndex); |
I think this implementation could be updated to fix the bug you found by calculating the page index within this callback...
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.
@davismcphee , i do not think calculating there is needed, because onChangePage
is not called when we need new pageIndex
to be calculated. The other useEffect
should be enough and i have included that.
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.
Also, I think onUpdatePageIndex
should now be solely based on currentPageIndex
. Any changes to that should be automatically trigger onUpdatePageIndex
instead of calling onUpdatePageIndex
at multiple places.
pageIndex: pagination.pageIndex > pageCount - 1 ? 0 : pagination.pageIndex, | ||
pageSize: pagination.pageSize, | ||
pageSizeOptions: rowsPerPageOptions ?? getRowsPerPageOptions(pagination.pageSize), | ||
pageIndex: currentPageIndex > pageCount - 1 ? 0 : currentPageIndex, |
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.
pageIndex: currentPageIndex > pageCount - 1 ? 0 : currentPageIndex, | |
pageIndex: currentPageIndex, |
...Then passing currentPageIndex
directly here.
And we'd only need to manually sync when pageCount
changes in a separate effect:
useEffect(() => {
setCurrentPageIndex((previousPageIndex) => {
const calculatedPageIndex = previousPageIndex > pageCount - 1 ? 0 : previousPageIndex;
if (calculatedPageIndex !== previousPageIndex) {
onUpdatePageIndex?.(calculatedPageIndex);
}
return calculatedPageIndex;
});
}, [onUpdatePageIndex, pageCount]);
Summary
This PR cleans up the pagination handling in Unified Data Table to make the logic easier to follow and less dependent on side effects.