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

[material-ui][TablePagination] Implement slotProps pattern for the actions and the select slots #39353

Conversation

anle9650
Copy link
Contributor

@anle9650 anle9650 commented Oct 8, 2023

Resolves #39336

@mui-bot
Copy link

mui-bot commented Oct 8, 2023

Netlify deploy preview

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

@material-ui/core: parsed: +0.08% , gzip: +0.08%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against fedbde3

@anle9650 anle9650 marked this pull request as ready for review October 9, 2023 02:05
@anle9650 anle9650 changed the title [TablePagination] Ability to show, but disable, the first/last page buttons [TablePagination] Ability to disable all (first, last, next, and previous) page buttons Oct 9, 2023
@anle9650 anle9650 changed the title [TablePagination] Ability to disable all (first, last, next, and previous) page buttons [material-ui][TablePagination] Ability to disable all (first, last, next, and previous) page buttons Oct 9, 2023
@anle9650 anle9650 changed the title [material-ui][TablePagination] Ability to disable all (first, last, next, and previous) page buttons [material-ui][TablePagination] Ability to disable all (first, last, next, previous) page buttons Oct 9, 2023
@cjenkscybercom
Copy link

cjenkscybercom commented Oct 9, 2023

Awesome! Could this also disable the page size dropdown as well?
(#39336)

@anle9650
Copy link
Contributor Author

anle9650 commented Oct 9, 2023

^ Done!

@anle9650 anle9650 changed the title [material-ui][TablePagination] Ability to disable all (first, last, next, previous) page buttons [material-ui][TablePagination] Ability to disable all (first, last, next, previous, select) page buttons Oct 9, 2023
@cjenkscybercom
Copy link

Awesome, thank you!

@zannager zannager added the component: TablePagination The React component. label Oct 10, 2023
@DiegoAndai
Copy link
Member

Hey @anle9650! Thanks for working on this 😊

I think it's a good addition 🎉

May I ask you to also add the firstIconButtonProps and lastIconButtonProps for completion? That way, we also support disabling those specifically.

@anle9650
Copy link
Contributor Author

@DiegoAndai just added those!

@DiegoAndai DiegoAndai added the package: material-ui Specific to @mui/material label Oct 12, 2023
@DiegoAndai
Copy link
Member

Thanks @anle9650!

I've discussed this with some members of the team, and we would prefer to implement the slotProps prop instead of adding firstIconButtonProps and lastIconButtonProps, I'm sorry for not bringing this up before. It would be a little more work, but the slotProps pattern is the standard we're trying to introduce (see this Slider slotProps example).

We would have to add the slotProps prop with keys firstIconButton, lastIconButton, nextIconButton, previousIconButton, and select. Using the first icon as an example:

  • slotProps = { firstIconButton: someProps } would be equivalent to firstIconButtonProps = {someProps}
  • slotProps.firstIconButton should override firstIconButtonProps
  • We would add a deprecation message to firstIconButtonProps: This prop is an alias for slotProps.firstIconButton and will be overridden by it if both are used (see this Popover PaperProps deprecation message example)

Do you have time to work on it? I can provide guidance for it 😊

Also, I want to bring @cherniavskii into the conversation as the DataGrid owner, as these API changes would impact the DataGrid's pagination prop:

  • What do you think about adding the global disabled prop to the pagination?
  • What do you think about introducing slotProps and deprecating the *IconButtonProps props?

@cherniavskii
Copy link
Member

Hi @DiegoAndai,
Looks good to me!
The Data Grid doesn't use the props you proposed to deprecate, and it's backward compatible anyway 👍🏻

@anle9650
Copy link
Contributor Author

@DiegoAndai just made those changes!

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Nice @anle9650! thanks

I think we're only missing a couple details, almost there 🚀

@DiegoAndai
Copy link
Member

DiegoAndai commented Oct 17, 2023

@anle9650 nice work! This is ready to merge from my perspective. Because I helped guide the solution, I'll ask for a second review from @mnajdova 🙌🏼

@anle9650 anle9650 force-pushed the Ability-to-show,-but-disable,-the-first/last-page-buttons branch from b53c20a to 751aab9 Compare October 20, 2023 02:07
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Hey @anle9650! thanks for keeping working on this

I added a couple more comments/questions to ensure that we don't break developer's apps with this change 😊

@anle9650 anle9650 force-pushed the Ability-to-show,-but-disable,-the-first/last-page-buttons branch from 4dcf345 to aff5479 Compare October 28, 2023 21:15
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the effort!

@mnajdova mnajdova merged commit a545acd into mui:master Nov 7, 2023
23 checks passed
@mnajdova mnajdova changed the title [material-ui][TablePagination] implement slotProps pattern for first, last, next, previous, select props [material-ui][TablePagination] Implement slotProps pattern for the actions and the select slots Nov 7, 2023
fzaninotto added a commit to marmelab/react-admin that referenced this pull request Nov 24, 2023
Material-ui v5.15.18 introduced a new prop injected to `PaginationAction`, `slotProps`, which isn't supposed to be passed down to the `Pagination` component.

Refs mui/material-ui#39353
Closes `<Pagination>` logs a warning #9470
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: TablePagination The React component. package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][TablePagination] Ability to show, but disable, the first/last page buttons
7 participants