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

[material-ui][AvatarGroup] deprecate componentsProps for v6 #42122

Open
wants to merge 22 commits into
base: next
Choose a base branch
from

Conversation

lhilgert9
Copy link
Contributor

Part of #41279
@DiegoAndai

@mui-bot
Copy link

mui-bot commented May 4, 2024

@lhilgert9 lhilgert9 changed the title [material-ui][AvatarGroup] deprecate *componentsProps for v6 [material-ui][AvatarGroup] deprecate componentsProps for v6 May 4, 2024
@zannager zannager added the component: avatar This is the name of the generic UI component, not the React module! label May 6, 2024
@zannager zannager requested a review from DiegoAndai May 6, 2024 16:19
Copy link
Member

@DiegoAndai DiegoAndai left a 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!

packages/mui-codemod/README.md Show resolved Hide resolved
packages/mui-material/src/AvatarGroup/AvatarGroup.d.ts 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;
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 implement the additionalAvatar slot properly? I think surplus is a better name. This would mean:

  • Providing slots.surplus and slotProps.surplus
  • Using useSlot hook
  • Adding tests for the slot

What do you think @lhilgert9?

@mnajdova tagging you as the owner of the Avatar component.

Copy link
Contributor Author

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.👍🏼

Copy link
Member

Choose a reason for hiding this comment

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

Let's implement it 🙌🏼

Copy link
Contributor Author

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.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 8, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 8, 2024
Copy link
Contributor Author

@lhilgert9 lhilgert9 left a 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.

Copy link
Member

@DiegoAndai DiegoAndai left a 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.

@lhilgert9
Copy link
Contributor Author

@DiegoAndai everything should be implemented. Tests failed because docker was down and the test images could not be pulled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: avatar This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants