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

[Theme] Merge components and slots props #35477

Merged
merged 4 commits into from
Dec 20, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Dec 14, 2022

close #34214

@siriwatknp siriwatknp added package: material-ui Specific to @mui/material package: joy-ui Specific to @mui/joy customization: theme Centered around the theming features labels Dec 14, 2022
@mui-bot
Copy link

mui-bot commented Dec 14, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-35477--material-ui.netlify.app/

@material-ui/core: parsed: +0.06% , gzip: +0.07%
@material-ui/lab: parsed: +0.11% , gzip: +0.13%
@material-ui/system: parsed: +0.42% , gzip: +0.50%
@material-ui/utils: parsed: +3.14% , gzip: +2.94%
@mui/material-next: parsed: +0.22% , gzip: +0.25%

Details of bundle changes

Generated by 🚫 dangerJS against 5713023

it('merge components and componentsProps props', () => {
expect(
resolveProps(
{ components: { Input: 'Input' }, componentsProps: { input: 'input' } },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ components: { Input: 'Input' }, componentsProps: { input: 'input' } },
{ components: { Input: 'Input' }, componentsProps: { input: { prop: 'input' } } },

This is a more valid use-case, same for the slotProps.

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I think I got your point.

const output = { ...props };

Object.keys(defaultProps).forEach((propName: keyof T) => {
if (output[propName] === undefined) {
if (propName.toString().match(/^(components|componentsProps|slots|slotProps)$/)) {
output[propName] = {
Copy link
Member

Choose a reason for hiding this comment

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

For the slotProps & componentsProps, should we merge the second level too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's better.

Comment on lines +24 to +26
const defaultSlotProps = (defaultProps[propName] || {}) as T[keyof T];
const slotProps = props[propName] as {} as T[keyof T];
output[propName] = {} as T[keyof T];
Copy link
Member Author

Choose a reason for hiding this comment

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

TypeScript is hard to get right for logic like this 🥲

@siriwatknp
Copy link
Member Author

@mnajdova Ready for another round of review.

...(defaultProps[propName] as any),
...(output[propName] as any),
};
} else if (propName.toString().match(/^(componentsProps|slotProps)$/)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we also merge props like TransitionProps (just by one depth lile slots and components ?
To solve #34978

Or should it be a distinct PR ?

Copy link
Member Author

@siriwatknp siriwatknp Dec 19, 2022

Choose a reason for hiding this comment

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

I think it should be a separate PR. I propose to keep it small to fix the main issue first.

packages/mui-utils/src/resolveProps.test.ts Show resolved Hide resolved
@siriwatknp
Copy link
Member Author

@mnajdova @flaviendelangle Should we consider this as a new feature (minor release) or a patch?

@flaviendelangle
Copy link
Member

No strong opinion, I'll let @mnajdova answer 👍

@siriwatknp siriwatknp merged commit 37d7df1 into mui:master Dec 20, 2022
@mnajdova
Copy link
Member

@mnajdova @flaviendelangle Should we consider this as a new feature (minor release) or a patch?

This is how it should have worked anyways, so I would consider it a bug fix, so a patch should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customization: theme Centered around the theming features package: joy-ui Specific to @mui/joy package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slots overrides do not merge with the theme
4 participants