-
-
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] Support advanced server-side pagination use cases #12474
[DataGrid] Support advanced server-side pagination use cases #12474
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Deploy preview: https://deploy-preview-12474--material-ui-x.netlify.app/ Updated pages: |
This comment was marked as outdated.
This comment was marked as outdated.
packages/x-data-grid/src/hooks/features/pagination/useGridRowCount.ts
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/hooks/features/pagination/useGridRowCount.ts
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/hooks/features/pagination/useGridRowCount.ts
Outdated
Show resolved
Hide resolved
docs/data/data-grid/pagination/ServerPaginationGridEstimated.tsx
Outdated
Show resolved
Hide resolved
docs/data/data-grid/pagination/ServerPaginationGridEstimated.tsx
Outdated
Show resolved
Hide resolved
docs/data/data-grid/pagination/ServerPaginationGridNoRowCount.tsx
Outdated
Show resolved
Hide resolved
docs/data/data-grid/pagination/ServerPaginationGridNoRowCount.tsx
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/hooks/features/pagination/useGridRowCount.ts
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/hooks/features/pagination/gridPaginationUtils.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com> Signed-off-by: Bilal Shafi <bilalshafidev@gmail.com>
if (!estimated) { | ||
return `${from}–${to} of ${count !== -1 ? count : `more than ${to}`}`; | ||
} | ||
return `${from}–${to} of ${count !== -1 ? count : `more than ${estimated > to ? estimated : to}`}`; |
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.
Would it work if instead of wrapping we pass different to
value depending on situation?
to: estimated > to ? estimated : to
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.
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.
Oh, got it.
In that case how about we fork TablePagination translations and move them to our locales? Then we can adapt labelDisplayedRows
to work with every locale.
What do you think?
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.
That's an interesting point.
I initially thought a bit further. i.e. forking the whole TablePagination
component on our side instead of translations only.
But there were a few reasons why I decided not to go with that approach at that point.
- The proper fix would be to do that on the core side since the component belongs in the core. If we move it to the X side, we may miss future updates on the core side (same with the translations)
- The estimated row count use case doesn't target a huge number of users, we did have a few requests for its support but I doubt that it will be demanded soon by many users, so is the effort even worth it?
I concluded that wrapping could be a nice middle ground. Meanwhile, we could add this functionality on the core side as Flavien suggested, and when after a major release, we bump the peer dependency on the core we remove this workaround.
Another side of the coin is the material-less grid which we are working on. That makes the argument of completely moving the component (TablePagination
) on the Grid side a bit stronger, but again there comes a discussion about the implementation strategy of the material-less version since we haven't yet figured out how the isolation between material and material-less versions will work. CC @romgrk (Correct me if I am wrong)
Anyhow, if it looks OK to you, we can proceed with the current workaround for now and settle on something better later?
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.
we could add this functionality on the core side as Flavien #12474 (comment), and when after a major release, we bump the peer dependency on the core we remove this workaround.
This sounds good to me. I'm not sure bumping the peer dependency on the core is a good idea though, since it would force users to upgrade even if they don't need this. Perhaps a better option would be showing a warning in the docs with the minimum supported version of @mui/material
required for this to work.
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.
I'm not sure bumping the peer dependency on the core is a good idea though
My proposal is:
- To implement something on X that do not require bumping the peer dep
- To have the core support whatever we think is the best DX, if this best DX requires a change on the core
- To bump the peer dep on the next major (we will probably need it for other topics anyway)
|
||
const defaultLabelDisplayedRows: WrappedLabelDisplayedRows = ({ from, to, count, estimated }) => { | ||
if (!estimated) { | ||
return `${from}–${to} of ${count !== -1 ? count : `more than ${to}`}`; |
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.
Shouldn't this sentence be in the localization?
Even if people can override it, that way people just have to pass there usual locale (which they probably already do) and they have a translated app.
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.
Yes, ideally estimated
should be supported by the @mui/material/TablePagination
(since that's where its localization is coming from). It would also mean a minimum peer dependency bump apart from adding the support in that component.
Until feasible, we can provide this workaround to our users to achieve the localization for the estimated row count use case. See the info callout just above this docs section.
Or your suggestion is more about moving localization for this function (labelDisplayedRows
) to the Grid localization already?
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.
I was thinking about the grid localization, but it's true that it could also belong to the TablePagination
one...
IMHO if we plan to put it in TablePagination
, then we could create the PR in the core repo to make sure that we have it for our next major.
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.
Final feedback on the docs mostly to follow our Writing style guide and make the docs more compact.
Thanks for working on it, it's a great addition to the server-side pagination! 🚀
Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com> Signed-off-by: Bilal Shafi <bilalshafidev@gmail.com>
Extracted from #12317
Resolves #409
Partially implements #2843
Fixes #4323
Preview: https://deploy-preview-12474--material-ui-x.netlify.app/x/react-data-grid/pagination/#index-based-pagination
Changelog