-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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][AvatarGroup] deprecate componentsProps
for v6
#42122
Conversation
Netlify deploy previewBundle size reportDetails of bundle changes (Toolpad) |
*componentsProps
for v6componentsProps
for v6
This reverts commit 9174052.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @lhilgert9, thanks for working on this!
docs/data/material/migration/migrating-from-deprecated-apis/migrating-from-deprecated-apis.md
Outdated
Show resolved
Hide resolved
@@ -105,7 +105,7 @@ const AvatarGroup = React.forwardRef(function AvatarGroup(inProps, ref) { | |||
const extraAvatars = Math.max(totalAvatars - clampedMax, totalAvatars - maxAvatars, 0); | |||
const extraAvatarsElement = renderSurplus ? renderSurplus(extraAvatars) : `+${extraAvatars}`; | |||
|
|||
const additionalAvatarSlotProps = slotProps.additionalAvatar ?? componentsProps.additionalAvatar; | |||
const additionalAvatarSlotProps = slotProps.additionalAvatar ?? componentsProps?.additionalAvatar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we implement the additionalAvatar
slot properly? I think surplus
is a better name. This would mean:
- Providing
slots.surplus
andslotProps.surplus
- Using
useSlot
hook - Adding tests for the slot
What do you think @lhilgert9?
@mnajdova tagging you as the owner of the Avatar component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds logical to me and would also ensure uniformity for all components and offer the end user more customization options.👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's implement it 🙌🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DiegoAndai I have added surplus
as slots
and slotProps
prop. I have marked slotProps.additionalAvatar
as @deprecated
. I also modified the codemod to change additionalAvatar
to surplus
. I just didn't know which tests you wanted to implement. If you tell me which ones we need there, I'll implement them too.
Co-authored-by: Diego Andai <diego@mui.com> Signed-off-by: Lucas Hilgert <77863078+lhilgert9@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The surplus slot has the same problem with overwriting the ownerState
as described in #42184. These lines were only inserted to solve the problem in the short term, but would have to be removed again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing the suggestion @lhilgert9!
May I ask you to add the slot test for the surplus
slot, it should be something like:
diff --git a/packages/mui-material/src/AvatarGroup/AvatarGroup.test.js b/packages/mui-material/src/AvatarGroup/AvatarGroup.test.js
index b646ad3fcf..0d9a26e7e7 100644
--- a/packages/mui-material/src/AvatarGroup/AvatarGroup.test.js
+++ b/packages/mui-material/src/AvatarGroup/AvatarGroup.test.js
@@ -22,14 +22,28 @@ describe('<AvatarGroup />', () => {
refInstanceof: window.HTMLDivElement,
testVariantProps: { max: 10, spacing: 'small', variant: 'square' },
testLegacyComponentsProp: true,
+ slots: {
+ surplus: { expectedClassName: classes.avatar },
+ },
+ skip: ['componentsProp'],
+ }),
+ );
+
+ // test additionalAvatar slot separately
+ describeConformance(
+ <AvatarGroup max={2}>
+ <Avatar src="/fake.png" />
+ <Avatar src="/fake.png" />
+ <Avatar src="/fake.png" />
+ </AvatarGroup>,
+ () => ({
+ classes,
+ render,
+ muiName: 'MuiAvatarGroup',
slots: {
additionalAvatar: { expectedClassName: classes.avatar },
},
- skip: [
- 'componentsProp',
- 'slotsProp',
- 'slotPropsCallback', // not supported yet
- ],
+ only: ['slotPropsProp'],
}),
);
We have to separate additionalAvatar
as it's only supported in slotProps
and not slots
.
packages/mui-codemod/src/deprecations/avatar-group-props/avatar-group-props.js
Show resolved
Hide resolved
packages/mui-codemod/src/deprecations/avatar-group-props/avatar-group-props.js
Show resolved
Hide resolved
@DiegoAndai everything should be implemented. Tests failed because docker was down and the test images could not be pulled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lhilgert9!
Part of #41279
@DiegoAndai