Skip to content
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

[docs][material-ui][Pagination] Clarify pagination page prop API #42220

Merged
merged 7 commits into from
May 16, 2024

Conversation

Mandar-Pandya
Copy link
Contributor

@Mandar-Pandya Mandar-Pandya commented May 13, 2024

Description

The current documentation for pagination api https://mui.com/material-ui/api/pagination/ is showing a bit of unclear description for the page attribute as it is not mentioned that is it a zero based index or not.

This is confusing because a similar api https://mui.com/material-ui/api/table-pagination/ where the same attribute is there and it is mentioned that it is a zero-based index and it might be confusing to not mentioned the same thing for pagination api i.e. it is not a zero based index.

Preview: https://deploy-preview-42220--material-ui.netlify.app/material-ui/api/pagination/#pagination-prop-page

Signed-off-by: Mandar-Pandya <112475099+Mandar-Pandya@users.noreply.github.com>
@mui-bot
Copy link

mui-bot commented May 13, 2024

Netlify deploy preview

https://deploy-preview-42220--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against b94d3b3

@zannager zannager added docs Improvements or additions to the documentation component: pagination This is the name of the generic UI component, not the React module! labels May 13, 2024
@zannager zannager requested a review from siriwatknp May 13, 2024 11:10
@Mandar-Pandya Mandar-Pandya changed the title docs: add clarifying info for pagination api [material-ui][docs] Add clarifying info for pagination api May 13, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mandar-Pandya Thanks for the pull request. I agree that the information should be included in the API prop description, as it feels missing. We do have a note about this in a section here:

Note that the Pagination page prop starts at 1 to match the requirement of including the value in the URL, while the TablePagination page prop starts at 0 to match the requirement of zero-based JavaScript arrays that come with rendering a lot of tabular data.

However, it would be helpful to also mention it in the API docs.

I suggest adding the following description:

The current page. The page starts at 1 unlike TablePagination which starts at 0.

Additionally, you'll need to update this in the usePagination type definition file here and then run the following commands:

pnpm proptypes
pnpm docs:api

This will update the PropTypes and the docs, and fix the CI. Please refer to the Contributing guide.

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][docs] Add clarifying info for pagination api [docs][material-ui][Pagination] Clarify pagination page prop API May 14, 2024
@ZeeshanTamboli ZeeshanTamboli added the package: material-ui Specific to @mui/material label May 14, 2024
Signed-off-by: Mandar-Pandya <112475099+Mandar-Pandya@users.noreply.github.com>
Signed-off-by: Mandar-Pandya <112475099+Mandar-Pandya@users.noreply.github.com>
@ZeeshanTamboli ZeeshanTamboli added the needs cherry-pick The PR should be cherry-picked to master after merge label May 16, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mandar-Pandya Thanks for the improvement! I made a slight tweak to the sentence.

@ZeeshanTamboli ZeeshanTamboli merged commit 7882099 into mui:next May 16, 2024
22 checks passed
github-actions bot pushed a commit that referenced this pull request May 16, 2024
…42220)

Signed-off-by: Mandar-Pandya <112475099+Mandar-Pandya@users.noreply.github.com>
Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pagination This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation needs cherry-pick The PR should be cherry-picked to master after merge package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants