[material-ui][TableSortLabel] Introduce slots and slotProps #42157
Conversation
sai6855
added
component: table
Netlify deploy previewTableSortLabel: parsed: +2.56% , gzip: +2.23% Bundle size reportDetails of bundle changes (Toolpad) |
sai6855
force-pushed
the
add-slots-tablesortlabel
branch
from
615da9d to
e256e02
Compare
github-actions
bot
added
the
PR: out-of-date
| const root = j(file.source); | ||
| const printOptions = options.printOptions; | ||
|
|
||
| replaceComponentsWithSlots(j, { root, componentName: 'TableSortLabel' }); |
| className={clsx(classes.icon)} | ||
| ownerState={ownerState} | ||
| /> | ||
| <TableSortLabelIcon as={IconSlot} className={clsx(classes.icon)} {...iconProps} /> |
There was a problem hiding this comment.
Shouldn't this be:
const externalForwardedProps = {
slots: {
icon: IconComponent,
...slots
},
slotProps,
};
const [IconSlot, iconProps] = useSlot('icon', {
elementType: TableSortLabelIcon,
externalForwardedProps,
ownerState,
className: classes.icon
});
// ...
<IconSlot {...iconProps} />There was a problem hiding this comment.
I just realized we did the same in https://github.com/mui/material-ui/pull/41875/files#diff-d626958b0fab135d0026b2cf86b08f733dae8abb98fc406df247b3846dcf8589R651
We will need to fix that 🤔
There was a problem hiding this comment.
are you sure?
By going with above approach TableSortLabelIcon won't be in consideration by below logic and it's styles won't be applied
slots[name] (here: IconComponent) preceeds over initialElementType ( here: TableSortLabelIcon)
There was a problem hiding this comment.
Oh, I see what's happening. There are some *Component props that are used as
<DefaultComponent
as={ComponentProp}
/>But others are used as
const { ComponentProp: DefaultComponent } = props
// ...
<ComponentProp />Which is different.
If I'm not mistaken, the convention for the slot pattern is to work as the latter: When you provide a slot, it replaces the default completely. Is this correct @siriwatknp?
There was a problem hiding this comment.
Other components with the as behavior:
There was a problem hiding this comment.
I assume codemod should also do same?
Yes, but let's wait for confirmation
There was a problem hiding this comment.
Let's wait for @siriwatknp's response, but my proposal would be:
- Deprecate
IconComponentand keep providing it throughas, so we keep the same functionality.- Implement
slots.iconso that it replaces the slot completely, following the pattern- In the deprecation message, the correct replacement for
IconComponentwould beslotProps.icon.as
Agree with the first two. I don't think the last is needed. Currently, we don't document as prop for Material UI, only component prop.
There was a problem hiding this comment.
I don't think the last is needed. Currently, we don't document as prop for Material UI, only component prop.
But what should we say the replacement for IconComponent is? Options:
- We could say the replacement is
slots.iconbut it's not the same (replaces entire slot) - We could implement the
iconprop which behaves asIconComponentdoes right now. I think it's a common-enough case to change the icon to justify a dedicated prop.
There was a problem hiding this comment.
Hey @DiegoAndai @siriwatknp any discussion happened on this PR front ?
There was a problem hiding this comment.
Sorry for the delay @sai6855. I think this is a special case (the same for other IconComponent props in the codebase). Let's not deprecate.
github-actions
bot
removed
the
PR: out-of-date
|
@DiegoAndai |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
added
PR: out-of-date
|
Hey @sai6855! thanks for working on this. Please see #41281 (comment). |
part of #41281
What does this PR do
slotsandslotPropsIconComponentNote: There are no
slotsrelated tests inskippedarray inTableSortLabel.test.js, so i didn't touched that file