[material-ui][Tabs] Deprecate *Props and complete slots, slotProps
#45012
Conversation
siriwatknp
added
the
component: tabs
Netlify deploy previewTabList: parsed: +1.21% , gzip: +1.15% Bundle size reportDetails of bundle changes (Toolpad) |
| }, | ||
| indicator: { | ||
| expectedClassName: classes.indicator, | ||
| }, |
There was a problem hiding this comment.
i think tests for scrollButton, startScrollButtonIcon, endScrollButtonIcon are missing
There was a problem hiding this comment.
Added at the end of this file. I could not leverage describeConformance because there are 2 scroll buttons when rendered, so the tests will fail due to queryByTestId.
| elementType: FlexContainer, | ||
| externalForwardedProps, | ||
| ownerState, | ||
| getSlotProps: (handlers) => ({ |
There was a problem hiding this comment.
Why use getSlotProps here and not with other handlers?
Where do the handlers.onKeyDown?.(event) come from? If it is from the props, then can we extract it and handle this on the handleKeyDown definition?
There was a problem hiding this comment.
Why use getSlotProps here and not with other handlers?
Because only flexContainer slot has onKeyDown logic in this component.
Where do the handlers.onKeyDown?.(event) come from
Yes, it comes from slotProps.flexContainer
The intention of the change solely due to slotProps.* could be a function. That's why getSlotProps exists from MUI Base to ensure that handlers is resolved to an object.
There was a problem hiding this comment.
Because only flexContainer slot has onKeyDown logic in this component.
But there are other event handlers, for example onChange on the scroller slot 🤔
There was a problem hiding this comment.
I guess you meant scrollbar slot and I think your question is "why scrollbar does not have getSlotProp given it receive onChange" right?
Let me update it.
| }, | ||
| }); | ||
|
|
||
| const [FlexContainerSlot, flexContainerSlotProps] = useSlot('flexContainer', { |
There was a problem hiding this comment.
The flexContainer slot seems difficult to understand from the users perspective, would 'list' be a better name?
There was a problem hiding this comment.
I don't think so, this is because the slot is created with flexContainer name. It's not a new slot.
// line: 198
const FlexContainer = styled('div', {
name: 'MuiTabs',
slot: 'FlexContainer',
overridesResolver: (props, styles) => {
const { ownerState } = props;
return [
styles.flexContainer,
ownerState.vertical && styles.flexContainerVertical,
ownerState.centered && styles.centered,
];
},
})({So styleOverrides already work with flexContainer slot. I think slots and slotProps should follow the same name.
There was a problem hiding this comment.
Could we deprecate styles.flexContainer and change it? I really think it's a bad name as it doesn't explain to users what this slot's role is in the component: flexContainer can be anything.
There was a problem hiding this comment.
Are you sure? that will increase the effort for writing a codemod to migrate styleOverrides. Given there is no complain about the flexContainer name so far, do you think it's worth to rename it @aarongarciah ?
There was a problem hiding this comment.
I agree it's not a good name. Let's change it now that we are at it 🙏
There was a problem hiding this comment.
@DiegoAndai let's do this in v7. Renaming flexContainer to list for the styleOverrides altogether now will be a breaking change.
docs/data/material/migration/migrating-from-deprecated-apis/migrating-from-deprecated-apis.md
Show resolved
Hide resolved
part of #41281
root, scroller, flexContainer, scrollbar, indicator, scrollButtons, startScrollButtonIcon, endScrollButtonIcon.These are existing slots and classes.