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] Add DefaultPropsProvider for Pigment CSS integration #42638

Merged
merged 24 commits into from
Jun 20, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 13, 2024

closes #42635

The gist is https://github.com/mui/material-ui/pull/42638/files#diff-532de7437125015b5ab87338e5e5950bc513e99fc7d987838a0f3755ee01408b, the rests are replacing createUseThemeProps with useDefaultProps.

The signature changes but still support v5:

<DefaultPropsProvider
  value={{
    MuiButton: {
      disableTouchRipple: true,
    }
  }}
>

Testing

Only the spec is added because the logic is already tested by describeConformance from all the components.


@siriwatknp siriwatknp added package: material-ui Specific to @mui/material v6.x migration labels Jun 13, 2024
@siriwatknp siriwatknp changed the title [material-ui] Add PropsProvider for Pigment CSS [material-ui] Add PropsProvider for Pigment CSS integration Jun 13, 2024
@mui-bot
Copy link

mui-bot commented Jun 13, 2024

Netlify deploy preview

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

DefaultPropsProvider: parsed: +Infinity% , gzip: +Infinity%
@material-ui/core: parsed: -0.08% 😍, gzip: -0.50% 😍
Pagination: parsed: -0.35% 😍, gzip: -0.81% 😍
PaginationItem: parsed: -0.36% 😍, gzip: -0.75% 😍
StepButton: parsed: -0.40% 😍, gzip: -0.66% 😍
AvatarGroup: parsed: -0.46% 😍, gzip: -0.85% 😍
DialogActions: parsed: -0.51% 😍, gzip: -1.02% 😍
AccordionSummary: parsed: -0.41% 😍, gzip: -0.69% 😍
AccordionActions: parsed: -0.51% 😍, gzip: -1.01% 😍
CardActions: parsed: -0.52% 😍, gzip: -1.01% 😍
Toolbar: parsed: -0.51% 😍, gzip: -1.00% 😍
StepLabel: parsed: -0.47% 😍, gzip: -0.82% 😍
DialogContentText: parsed: -0.52% 😍, gzip: -0.86% 😍
LoadingButton: parsed: -0.40% 😍, gzip: -0.46% 😍
DialogContent: parsed: -0.51% 😍, gzip: -0.98% 😍
FormGroup: parsed: -0.51% 😍, gzip: -0.98% 😍
ListItemIcon: parsed: -0.51% 😍, gzip: -0.97% 😍
BottomNavigationAction: parsed: -0.41% 😍, gzip: -0.66% 😍
ListSubheader: parsed: -0.50% 😍, gzip: -0.96% 😍
TableSortLabel: parsed: -0.41% 😍, gzip: -0.61% 😍
and 68 more changes

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against f2b658a

@zannager zannager requested a review from mnajdova June 13, 2024 16:29
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 14, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 17, 2024
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.

We have a bit of misalignment in the names:

  • PropsProvider vs useDefaultProps - we should either have DefaultPropsProvider and useDefaultProps, or PropsProvider and useProps

I like that the API is identical as before, but in ideal scenario this API in my opinion should be:

<DefaultPropsProvider
  value={{
    MuiButton: { disableRipple: true }, // note that I removed the defaultProps key
  }}
>

I would prefer this API ☝️

@siriwatknp siriwatknp changed the title [material-ui] Add PropsProvider for Pigment CSS integration [material-ui] Add DefaultPropsProvider for Pigment CSS integration Jun 19, 2024
@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 19, 2024

We have a bit of misalignment in the names:

  • PropsProvider vs useDefaultProps - we should either have DefaultPropsProvider and useDefaultProps, or PropsProvider and useProps

I like that the API is identical as before, but in ideal scenario this API in my opinion should be:

<DefaultPropsProvider
  value={{
    MuiButton: { disableRipple: true }, // note that I removed the defaultProps key
  }}
>

I would prefer this API ☝️

Changed to DefaultPropsProvider and support both signatures.

@siriwatknp siriwatknp requested a review from mnajdova June 19, 2024 04:07
@@ -1,3 +1,4 @@
.MuiTab-root .MuiTab-icon {
color: red;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not related to this PR. I have no idea how this passed the check before merging to next.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in #42686

}

if (!config.styleOverrides && !config.variants) {
// v6 signature, no property 'defaultProps'
Copy link
Member

Choose a reason for hiding this comment

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

Good call for adding both APIs 👌

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.

Nice work, let's merge this before the beta 🎉 This was the last missing part for interoperability, right?

@siriwatknp
Copy link
Member Author

Nice work, let's merge this before the beta 🎉 This was the last missing part for interoperability, right?

No. There are 2 more, left over components and Pigment layout components (need decision on mui/pigment-css#144 (comment))

@siriwatknp siriwatknp enabled auto-merge (squash) June 20, 2024 10:15
@siriwatknp siriwatknp merged commit 79f94eb into mui:next Jun 20, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui] Add PropsProvider for Pigment CSS integration
5 participants