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][PaginationItem] Deprecate components prop #41777

Merged
merged 26 commits into from
Apr 30, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Apr 5, 2024

part of #41279

@sai6855 sai6855 marked this pull request as draft April 5, 2024 04:54
@sai6855 sai6855 marked this pull request as ready for review April 5, 2024 04:54
@sai6855 sai6855 added docs Improvements or additions to the documentation deprecation New deprecation message package: material-ui Specific to @mui/material component: pagination This is the name of the generic UI component, not the React module! labels Apr 5, 2024
@mui-bot
Copy link

mui-bot commented Apr 5, 2024

Netlify deploy preview

Pagination: parsed: +2.74% , gzip: +2.57%
PaginationItem: parsed: +2.86% , gzip: +2.65%
packages/material-ui/material-ui.production.min.js: parsed: +0.08% , gzip: +0.12%
@material-ui/core: parsed: +0.07% , gzip: +0.07%
@material-ui/lab: parsed: +0.13% , gzip: +0.12%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 606f8c3

@sai6855 sai6855 marked this pull request as draft April 5, 2024 04:58
@sai6855
Copy link
Contributor Author

sai6855 commented Apr 8, 2024

Below tests are not removed , because describeConformance test suite is not customizable enough to handle PaginationItem.

'slotsProp',
'slotPropsProp',
'slotPropsCallback', // not supported yet

I see 2 inconsistencies in PaginationItem compare to other components.

  1. properties in components prop are always started in capital letter in other components but in PaginationItem it starts with small letter
  2. if slot is passed through components prop or slots prop it is guranteed to get rendered but in PaginationItem it's not guranteed that passed slot will be displayed as type prop should be equal to passed slot.

Due to these inconsistencies following tests are not passed ( there are many more, i didn't listed all) on removing slotsProp, slotPropsProp, slotPropsCallback from skip array

[capitalize(slotName)]: slotComponent,

[capitalize(slotName)]: ComponentForComponentsProp,

@sai6855 sai6855 requested a review from DiegoAndai April 8, 2024 06:37
@sai6855 sai6855 marked this pull request as ready for review April 8, 2024 06:37
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 12, 2024
@DiegoAndai
Copy link
Member

Hey @sai6855, thanks for working on this!

Regarding the tests, would removing testLegacyComponentsProp: true (here) fix this issue so we can stop skipping the slots tests?

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 15, 2024
@sai6855
Copy link
Contributor Author

sai6855 commented Apr 15, 2024

fix this issue so we can stop skipping the slots tests?

No, it didn't helped. even on removing testLegacyComponentsProp: true tests are failing.

@DiegoAndai
Copy link
Member

DiegoAndai commented Apr 16, 2024

Even on removing testLegacyComponentsProp: true tests are failing.

I see. Some tests can be fixed by removing testLegacyComponentsProp: true, but others are failing because the rendering of slots is conditional to the type: https://github.com/mui/material-ui/pull/41777/files#diff-014e80cab0c8e6df4ca350c7fd91d2d7b490250af715108435ed9113e1886216R413-R418

So slots are not always rendered, as describeConformance expects. @siriwatknp do you know of any other case of slots being rendered conditionally? If it's common enough we could cover it in describeConformance, otherwise, we can cover the slots testing in PaginationItem tests.

@sai6855
Copy link
Contributor Author

sai6855 commented Apr 23, 2024

otherwise, we can cover the slots testing in PaginationItem tests.

@DiegoAndai shall i go ahead with this approach?

@DiegoAndai
Copy link
Member

@DiegoAndai shall i go ahead with this approach?

Yes, let's do it. It makes sense to me to cover this custom case separately.

Let's add a comment pointing out that the slots tests are covered outside of describeConformance because of this.

@sai6855
Copy link
Contributor Author

sai6855 commented Apr 26, 2024

@DiegoAndai added tests to test conditional slots

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.

Thanks for updating the tests @sai6855, I have a couple of follow-up comments. Let me know what you think 😊

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.

Great job @sai6855!

@DiegoAndai DiegoAndai merged commit 02100b3 into mui:next Apr 30, 2024
22 checks passed
@sai6855 sai6855 deleted the deprecate-pagination-item-components branch April 30, 2024 18:49
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! deprecation New deprecation message docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants