Data grid Pagination

[Domino987]

Hello @oliviertassinari ,

this PR adds the pagination functionality to the data grid.

It saves the current page and pageSize and show the data with regard to changes made by the user.

It also lets the user to set the inital page, the page size and page size options.

What do you think?

[oliviertassinari]

Thanks, I was just starting looking at it :)

[oliviertassinari]

I was thinking of the following API:

    onRowsPerPageChange,
    onPageChange,
    pagination = false,
    paginationPage = 0,
    paginationPageSize = 50,
    paginationRowsPerPage,

[oliviertassinari]

I have pushed my latest changes.

[Domino987]

I will adjust the names.
Should the callbacks be in the paging prop object or at the top level of the prop? What do you prefer?

[oliviertassinari]

I was thinking of keeping as much as possible flat, at the top level (for the props).
A few more notes:

• search console's default pagination looks good:
  
• 50 rows per pages seem to be a common default among google products (gsuite admin, google ads, google cloud)
• we might not know the total number of rows (not with the current client-side data backend, but with the server-side one, that is left to be built), we need to support something like ("of more than"):
  
  google ads
• For the data provider's params, I think that we should provide arguments, so people can either implement offset based pagination or key-based one (to query the database).
• We need to allow people to control the pagination from the outside of the component.
• We will need to look at how we handle rows loading state, I have recently introduced a loading prop at the component level, but it's designed for the initial load or when sorting, filtering, not when lazy loading more rows.

If you want to take on the pagination, I will jump it to move to the filtering.

[Domino987]

Yes I will spend time on it . Thanks for the feedback.

[Domino987]

@oliviertassinari , I did more work on the pagination.
So now the pagination works for the basic cases and the props are flat.
I extended the data provider optionally with loadMoreRows. This was done since the rowData comes from the parent (so not controlled by us), which needs to be extended on if the last page is reached. While getList returns the currently displayed rows. To separate these, I decided to separate these as well. What do you think?
When the last page is reached and loadMoreRows is provided, it allows the user to update the local rows with the provided pagination key. It expects a new key to further paginate, if again the new last page is reached. If loadMoreRows is provided, the pagination text is also changed to reflect the additional loading.
I provided an example in the paging docs.
Let me know what you think.

[Domino987]

Hi wow that's a detailed code review. Thanks.
Few points I would like to discuss further:
To 5: I think we should make it either fully or partial controlled where we allow to set the initial page and still allow the user to change the page. We could split it with initial page and current page. This way, if the dev only wants to set the initial page, he does not have to implement the whole pagination logic. But if he wants to set page as static, he still could react to onPageChange.
To 10: I added a catch, but we need an error page or message to be displayed. Maybe show the snack bar with an error message.
To 11: Can you elaborate, what you would change. I do not know exactly what you mean.
To 13: These should be in the state so the user can change it, if the dev only wants to set the initial size/options. It's similar to page. Do we want to make it uncontrolled, set the Initial or make it fully controlled?
To 14: Which useEffect do you mean?
I will rebase tomorrow and push some changes.

[oliviertassinari]

• 5 Partially controlled props should be avoided as much as possible. We should almost forbid them. Here, I think that we should be lazy, making it uncontrolled. Later, if we have feature requests for it, we can add a controlled mode. So maybe we should rename the prop defaultPaginationPage?
• 10 I don't think that we should use the Snackbar yet it would have an impact on the whole app and increase bundle size for a one-off use case. What about we display an error at the bottom of the component with a release button?
• 11 I mean using () => f(x) when f never return anything. I propose () => { f(x) }.
• 13 Ah yes, you are right, paginationRowsPerPageOptions is the only one we can remove from the state. Users can modify it. However, regarding paginationPageSize or if we rename it paginationRowsPerPage (+1), users can change it, so yes, I guess it should support the controlled and uncontrolled modes. In this case, I think that we can start with the uncontrolled one, So maybe we should rename the prop defaultRowsPerPage?
• 14 I think that with 13, it will go away anyway: https://github.com/oliviertassinari/material-ui/pull/2/files#diff-276569f3138366d1a03a779e8af12599R385.

What about this new API proposal?

defaultRowsPerPage?: number;
pagination?: number;
paginationRowsPerPageOptions?: Array<number | { value: number; label: string }>;

// and later, unless you want to implement it directly :)
defaultPage?: number;
rowsPerPage?: number;
page?: number;
onRowsPerPageChange?: React.ChangeEventHandler<HTMLTextAreaElement | HTMLInputElement>;
onPageChange?: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;

[oliviertassinari]

Thanks for the synchronization of the branch with the main one :). I'm curious, did you consider to squash and rebase? Sometimes, I feel it's simpler, but we will see, maybe not.

[Domino987]

I failed the rebase...
I will open a new PR with the current changes.
I am sorry.

[oliviertassinari]

@Domino987 No problem, hold on. I think that we can continue working on it, then squash and rebase with GitHub. We can probably save time here :). Let's ignore my comment, for now.

[oliviertassinari]

@Domino987 Thanks for taking my feedback into account! I have pushed the changes one step further. Here is a summary:

• Reset the pagination after a sort
• Update the prop-types (yarn proptypes)
• Update the markdown (yarn docs:api)
• Remove the default value from the description, our pipeline is smart enough to pick the default value from the source.
• Scope the usage of rowsData to the default dataSource. If we use it elsewhere, it won't work once we move to a server-side implementation.
• Gave up on the cursor pagination, for now, to simplify the source
• Use two different states for two future potential controlled props (active page and number of displayed rows).
• Don't handle default props updates (ideally, we should warn)
• Add a cancel like request logic (active variable)
• Use the state to retry a request (so we can benefit from the effect cleanup logic)
• Introduce a concept of total number of records. It's very likely that developers wouldn't know the value. I remember having to query a table of 2m records, calculating the number of rows was requiring a scan of the whole table (with PostgreSQL) it was very slow, we had to use an approximation instead, with table stats.

Let me know what you think of the changes :).

As a follow-up, I think that it would be great to introduce the "of more" concept, as you can see in Google Ads or Ag-grid:

For the implementation, maybe a <TablePagination count={-1} /> API? Do you want to take care of it? :)