[Theme] Merge components and slots props #35477
Conversation
siriwatknp
added
package: material-ui
@material-ui/core: parsed: +0.06% , gzip: +0.07% |
| it('merge components and componentsProps props', () => { | ||
| expect( | ||
| resolveProps( | ||
| { components: { Input: 'Input' }, componentsProps: { input: 'input' } }, |
There was a problem hiding this comment.
| { 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.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
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] = { |
There was a problem hiding this comment.
For the slotProps & componentsProps, should we merge the second level too?
| const defaultSlotProps = (defaultProps[propName] || {}) as T[keyof T]; | ||
| const slotProps = props[propName] as {} as T[keyof T]; | ||
| output[propName] = {} as T[keyof T]; |
There was a problem hiding this comment.
TypeScript is hard to get right for logic like this 🥲
|
@mnajdova Ready for another round of review. |
| ...(defaultProps[propName] as any), | ||
| ...(output[propName] as any), | ||
| }; | ||
| } else if (propName.toString().match(/^(componentsProps|slotProps)$/)) { |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
I think it should be a separate PR. I propose to keep it small to fix the main issue first.
|
@mnajdova @flaviendelangle Should we consider this as a new feature (minor release) or a patch? |
|
No strong opinion, I'll let @mnajdova answer 👍 |
This is how it should have worked anyways, so I would consider it a bug fix, so a patch should be fine. |
close #34214