[material-ui][TablePagination] Add ability to change icons in TablePaginationActions using `slots` and `slotProps` #33797

pratikkarad · 2022-08-04T10:44:06Z

I have followed (at least) the PR section of the contributing guide.

I have tested this fix locally and also tested using a preview package on CodeSandbox.

Closes #33231

mui-bot · 2022-08-04T10:48:25Z

Netlify deploy preview

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

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against dd84443

propG · 2022-08-04T10:56:29Z

Thanks

…

On Thu, Aug 4, 2022, 5:49 AM MUI Bot ***@***.***> wrote: Bundle size will be reported once CircleCI build #413347 <https://app.circleci.com/pipelines/github/mui/material-ui/jobs/413347> finishes. Generated by 🚫 dangerJS <https://danger.systems/js> against 181edc0 <181edc0> — Reply to this email directly, view it on GitHub <#33797 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2KTIJ54JYFJ25GNFSZMBVTVXON33ANCNFSM55SBV7CQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

mnajdova

Please also add componentsProps for allowing people to specify additional props if needed on these slots.

mnajdova

Should we also support callbacks? cc @michaldudak

docs/pages/material-ui/api/table-pagination.json

michaldudak · 2022-08-16T06:47:43Z

Should we also support callbacks? cc @michaldudak

It's not crucial here (there is no real ownerState in TablePaginationActions component), but I suppose at some point we'd like to have components/componentProps in every component in Material UI and have it consistent with Base/Joy. So it doesn't have to be in this PR, but we'll need it sometime in the future.

siriwatknp · 2022-09-16T03:32:07Z

I agree that the icons should be customizable but I am not satisfied with the components prop. The components prop in this PR is inconsistent with others because it targets nested components. I put an on hold label until we have a conclusion on components override.

pratikkarad · 2022-10-01T13:57:53Z

Hi @siriwatknp, please update the PR with the approach once you have a conclusion on the components prop.

siriwatknp · 2022-10-03T07:15:59Z

Hi @siriwatknp, please update the PR with the approach once you have a conclusion on the components prop.

Sure! so sorry for the delay.

jake-collibra · 2022-10-31T14:56:19Z

This pattern is seen in DatePicker, and now Alert as well
https://mui.com/x/api/date-pickers/date-picker/
https://mui.com/material-ui/api/alert/

ZeeshanTamboli · 2023-03-10T10:28:13Z

Any updates on this? As far as I know, I think we finally decided to use slots and slotProps to override components now. @pratikkarad I am closing this PR. If you would like to update it, please re-open it and apply the changes.

pratikkarad · 2023-03-10T11:51:17Z

@ZeeshanTamboli I'll fix this.

ZeeshanTamboli

@pratikkarad I made the expected changes. Thank you for the PR.

@DiegoAndai Could you please review this?

DiegoAndai · 2023-12-08T13:46:10Z

@ZeeshanTamboli API design might be confusing, as the slots and slotProps values differ (i.e., the firstButton slot props and firstPageIcon slot), what do you think about the following:

Have the slots and slotProps be firstButton, lastButton, nextButton, previousButton
For changing only the icon (#33231's case), use the children slot props: slotProps={{ firstButton: { children: <CustomIcon /> }}
For further customization, use the slot: slots={{ firstButton: ... }}

Would that work?

ZeeshanTamboli · 2023-12-08T14:38:30Z

@ZeeshanTamboli API design might be confusing, as the slots and slotProps values differ (i.e., the firstButton slot props and firstPageIcon slot), what do you think about the following:

Have the slots and slotProps be firstButton, lastButton, nextButton, previousButton

For changing only the icon (#33231's case), use the children slot props: slotProps={{ firstButton: { children: <CustomIcon /> }}

For further customization, use the slot: slots={{ firstButton: ... }}

Would that work?

@DiegoAndai, I considered using slots for buttons firstButton, lastButton, nextButton, and previousButton, similar to TablePaginationActions in MUI Base. However, I don't see the advantage of providing slots for buttons, as it's unlikely that someone would need a different element other than a button to be rendered there. If needed, users can use the component prop in slotProps like:

<TablePagination
  ....
  ....
  slotProps={{
    actions: {
      firstButton: {
        component: 'button', // HTML element or a React element
      },
    },
  }}
/>;

I found information about this here: #34334 (comment).

Yes, but the API design for properties differs between slots and slotProps, each having different properties for icons and buttons. However, is it necessary for slots and slotProps to be the same?

For changing only the icon (#33231 case), use the children slot props: slotProps={{ firstButton: { children: }}

(edited) ~~If we could recommended this, we don't need this PR then unless you still suggest having slots for buttons.~~ 👍
Oh, no, we'll still need to spread children.

DiegoAndai · 2023-12-08T15:04:17Z

However, is it necessary for slots and slotProps to be the same?

I wouldn't say it's necessary, but IMO, it's a clearer API design. I wouldn't break it unless strictly necessary. Pure speculation here: If the slots from the slots prop and those from the slotProps prop differ, then the mental model of the component's internal structure becomes confusing.

Oh, no, we'll still need to spread children.

Yes, what do you think about doing that instead?

ZeeshanTamboli · 2023-12-09T08:45:58Z

Yes, what do you think about doing that instead?

@DiegoAndai To be honest, I don't like it. It's too nested: slotProps.actions.firstButton.children and I suppose we would need the children to be React.Node instead of React.Element.

Instead, I propose slots firstButton, lastButton, nextButton, previousButton, firstPageIcon, lastPageIcon, nextPageIcon and previousPageIcon for both slots and slotsProp props. What do you think?

DiegoAndai · 2023-12-11T18:57:02Z

@ZeeshanTamboli I guess the "rule" we could follow is to have slots for components instantiated inside the parent. If that's the "rule", we could have slots for the icons as those are instantiated in the TablePaginationActions component. I would use "Button" instead of "Page", though: firstButtonIcon instead of firstPageIcon.

So I would be on board with: firstButton, lastButton, nextButton, previousButton, firstButtonIcon, lastButtonIcon, nextButtonIcon and previousButtonIcon for both slots and slotsProp props.

A question that arises now is how these should behave regarding right-to-left mode. Would the user expect, for example, for nextButtonIcon and previousButtonIcon to be switched on right-to-left? Or would they expect these to stay on the same position?

Sidenote:

To be honest, I don't like it. It's too nested: slotProps.actions.firstButton.children

I realized that we made a mistake in that PR. It should be slotProps.actions.slotProps.firstButton.

ZeeshanTamboli · 2023-12-13T07:36:27Z

@ZeeshanTamboli I guess the "rule" we could follow is to have slots for components instantiated inside the parent. If that's the "rule", we could have slots for the icons as those are instantiated in the TablePaginationActions component. I would use "Button" instead of "Page", though: firstButtonIcon instead of firstPageIcon.

So I would be on board with: firstButton, lastButton, nextButton, previousButton, firstButtonIcon, lastButtonIcon, nextButtonIcon and previousButtonIcon for both slots and slotsProp props.

Alright, I'll make the changes.

A question that arises now is how these should behave regarding right-to-left mode. Would the user expect, for example, for nextButtonIcon and previousButtonIcon to be switched on right-to-left? Or would they expect these to stay on the same position?

Certainly, it should switch successfully with custom slots since it already functions without them: https://codesandbox.io/p/sandbox/inspiring-hofstadter-8pzmn9?file=%2Fsrc%2Findex.tsx%3A22%2C4.

Sidenote:

To be honest, I don't like it. It's too nested: slotProps.actions.firstButton.children

I realized that we made a mistake in that PR. It should be slotProps.actions.slotProps.firstButton.

Hmm, but if this is done now it would be a breaking change.

DiegoAndai · 2023-12-13T13:09:20Z

Hmm, but if this is done now it would be a breaking change.

Yes 😅 I was just noting it.

ZeeshanTamboli · 2023-12-13T16:41:13Z

@DiegoAndai This is ready for review. Also, here is the CodeSandbox demonstrating custom action slots with RTL mode.

DiegoAndai

Glad to get this one done 😊 good work 🎉

[TablePagination] Add ability to change icons

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

181edc0

pratikkarad added 3 commits August 4, 2022 16:59

[TablePagination] Add unit tests to check icons

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

153e99c

[TablePagination] Split unit tests

44d1ff8

[TablePagination] fix argos

30ff0a6

mnajdova reviewed Aug 10, 2022

View reviewed changes

pratikkarad added 2 commits August 14, 2022 21:08

[TablePagination] Add componentProps

92d908d

[TablePagination] Add unit tests for componentsProps

0521a10

pratikkarad requested a review from mnajdova August 14, 2022 16:12

mnajdova reviewed Aug 15, 2022

View reviewed changes

docs/pages/material-ui/api/table-pagination.json Outdated Show resolved Hide resolved

pratikkarad added 2 commits August 16, 2022 10:16

[TablePagination] Update components prop

f5e461f

[TablePagination] Update unit tests

ff2cf6b

github-actions bot added the PR: out-of-date label Aug 25, 2022

pratikkarad requested a review from mnajdova August 29, 2022 11:27

oliviertassinari requested review from hbjORbj and siriwatknp as code owners September 5, 2022 14:56

siriwatknp added the on hold label Sep 16, 2022

siriwatknp mentioned this pull request Oct 3, 2022

[RFC] The way to override components and slots #34334

Closed

ZeeshanTamboli closed this Mar 10, 2023

ZeeshanTamboli reopened this Mar 10, 2023

Merge branch 'master' into table-pagination-33231

37a7a63

github-actions bot removed the PR: out-of-date label Mar 10, 2023

add slots prop to override icons in TablePagintion component

a05ac46

github-actions bot removed the PR: out-of-date label Dec 7, 2023

ZeeshanTamboli added 4 commits December 7, 2023 19:48

prettier

b09516c

forward slots to TablePaginationActions

ce69f3e

add TS test

f352b5c

add test

93375fb

ZeeshanTamboli changed the title ~~[TablePagination] Add ability to change icons used in TablePaginationActions~~ [material-ui][TablePagination] Add ability to change icons used in TablePaginationActions Dec 8, 2023

ZeeshanTamboli added 2 commits December 8, 2023 12:01

update description

0728bbd

trigger CI

1fdf139

ZeeshanTamboli reviewed Dec 8, 2023

View reviewed changes

ZeeshanTamboli added the package: material-ui label Dec 8, 2023

ZeeshanTamboli added 6 commits December 13, 2023 21:11

add TS test

8233264

add unit tests

aab77c0

add logic, make the tests pass

d7574b1

generate API docs

4da4c62

Merge branch 'master' into table-pagination-33231

ebe6d4f

support rtl for action button slot props

dd84443

ZeeshanTamboli changed the title ~~[material-ui][TablePagination] Add ability to change icons used in TablePaginationActions~~ [material-ui][TablePagination] Add ability to change icons used in TablePaginationActions using slots and slotProps Dec 13, 2023

DiegoAndai approved these changes Dec 13, 2023

View reviewed changes

ZeeshanTamboli changed the title ~~[material-ui][TablePagination] Add ability to change icons used in TablePaginationActions using slots and slotProps~~ [material-ui][TablePagination] Add ability to change icons in TablePaginationActions using slots and slotProps Dec 14, 2023

ZeeshanTamboli merged commit a5d6da7 into mui:master Dec 14, 2023

DiegoAndai mentioned this pull request Feb 21, 2024

[material-ui][Table] slots and slotProps documented properties are not exposed in package #41205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[material-ui][TablePagination] Add ability to change icons in TablePaginationActions using `slots` and `slotProps` #33797

[material-ui][TablePagination] Add ability to change icons in TablePaginationActions using `slots` and `slotProps` #33797

pratikkarad commented Aug 4, 2022 •

edited by ZeeshanTamboli

Loading

mui-bot commented Aug 4, 2022 •

edited

Loading

propG commented Aug 4, 2022 via email

mnajdova left a comment

mnajdova left a comment

michaldudak commented Aug 16, 2022

siriwatknp commented Sep 16, 2022

pratikkarad commented Oct 1, 2022

siriwatknp commented Oct 3, 2022

jake-collibra commented Oct 31, 2022

ZeeshanTamboli commented Mar 10, 2023 •

edited

Loading

pratikkarad commented Mar 10, 2023

ZeeshanTamboli left a comment

DiegoAndai commented Dec 8, 2023

ZeeshanTamboli commented Dec 8, 2023 •

edited

Loading

DiegoAndai commented Dec 8, 2023

ZeeshanTamboli commented Dec 9, 2023 •

edited

Loading

DiegoAndai commented Dec 11, 2023

ZeeshanTamboli commented Dec 13, 2023 •

edited

Loading

DiegoAndai commented Dec 13, 2023

ZeeshanTamboli commented Dec 13, 2023

DiegoAndai left a comment

[material-ui][TablePagination] Add ability to change icons in TablePaginationActions using slots and slotProps #33797

[material-ui][TablePagination] Add ability to change icons in TablePaginationActions using slots and slotProps #33797

Conversation

pratikkarad commented Aug 4, 2022 • edited by ZeeshanTamboli Loading

mui-bot commented Aug 4, 2022 • edited Loading

Netlify deploy preview

Bundle size report

propG commented Aug 4, 2022 via email

mnajdova left a comment

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

michaldudak commented Aug 16, 2022

siriwatknp commented Sep 16, 2022

pratikkarad commented Oct 1, 2022

siriwatknp commented Oct 3, 2022

jake-collibra commented Oct 31, 2022

ZeeshanTamboli commented Mar 10, 2023 • edited Loading

pratikkarad commented Mar 10, 2023

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

DiegoAndai commented Dec 8, 2023

ZeeshanTamboli commented Dec 8, 2023 • edited Loading

DiegoAndai commented Dec 8, 2023

ZeeshanTamboli commented Dec 9, 2023 • edited Loading

DiegoAndai commented Dec 11, 2023

ZeeshanTamboli commented Dec 13, 2023 • edited Loading

DiegoAndai commented Dec 13, 2023

ZeeshanTamboli commented Dec 13, 2023

DiegoAndai left a comment

Choose a reason for hiding this comment

[material-ui][TablePagination] Add ability to change icons in TablePaginationActions using `slots` and `slotProps` #33797

[material-ui][TablePagination] Add ability to change icons in TablePaginationActions using `slots` and `slotProps` #33797

pratikkarad commented Aug 4, 2022 •

edited by ZeeshanTamboli

Loading

mui-bot commented Aug 4, 2022 •

edited

Loading

ZeeshanTamboli commented Mar 10, 2023 •

edited

Loading

ZeeshanTamboli commented Dec 8, 2023 •

edited

Loading

ZeeshanTamboli commented Dec 9, 2023 •

edited

Loading

ZeeshanTamboli commented Dec 13, 2023 •

edited

Loading