[Avatar] Migrate to emotion

[oliviertassinari]

It took me about 1 hour without knowing exactly what I was doing, I tried to copy the changes in #24107 😆 . We have about 150 components to migrate. So in theory, the migration effort could take about 1 month.

Proposals improvements, to solve pains I have faced during the effort:

1. Reduce the need to change theme import paths

const AvatarRoot = experimentalStyled(
  'div',
  {},
  {
    name: 'Avatar',
    slot: 'Root',
    overridesResolver,
  },
-)((props) => ({
+)(({ theme, styleProps }) => ({

This would have saved me some time to rename the props. I also think that it's easier to ready and more aligned with what we document.

2. Reduce prop name renaming, make it easier to navigate the code:

    <AvatarRoot
      as={Component}
-     styleProps={stateAndProps}
+     styleProps={styleProps}
      className={clsx(classes.root, className)}
      ref={ref}

-const useAvatarClasses = (props) => {
- const { classes = {}, variant, colorDefault } = props;
+const useAvatarClasses = (styleProps) => {
+ const { classes = {}, variant, colorDefault } = styleProps;

  const utilityClasses = {

I got confused about what was what.

3. I don't think that it makes sense to write .js and .d.ts for new files. It seems simpler to use .tsx directly. So I think that we should:

-Avatar/avatarClasses.d.ts
-Avatar/avatarClasses.js
+Avatar/avatarClasses.tsx

4. Do we need to name the returned value of overridesResolver? What about:

const overridesResolver = (props, styles) => {
  const { variant = 'circular' } = props;

- const styleOverrides = {
+ return {
    ...styles.root,
    ...styles[variant],
    [`&.${avatarClasses.img}`]: styles.img,
    [`&.${avatarClasses.fallback}`]: styles.fallback,
  };
-
- return styleOverrides;
};

Same question for no intermediate variable in useAvatarClasses.

5. Do we need to give useAvatarClasses a specific name? What about a generic one to make it easier to copy & paste?

-const useAvatarClasses = (props) => {
+const useUtilityClasses = (props) => {
  const { classes = {}, variant, colorDefault } = props;

  const utilityClasses = {
    root: clsx(avatarClasses.root, classes.root, getAvatarUtilityClass(variant), {
      [avatarClasses.colorDefault]: colorDefault,
    }),

6. Why is getAvatarUtilityClass public?
7. It seems that we could abstract avatarClasses.js to be less verbose. I believe that it's always the same. How about we have an array?

diff --git a/packages/material-ui/src/Avatar/avatarClasses.js b/packages/material-ui/src/Avatar/avatarClasses.js
index 5ede5b894e..2c8a4549da 100644
--- a/packages/material-ui/src/Avatar/avatarClasses.js
+++ b/packages/material-ui/src/Avatar/avatarClasses.js
@@ -1,15 +1,13 @@
-export function getAvatarUtilityClass(name) {
-  return `MuiAvatar-${name}`;
-}
+import generateUtilityClass from '@material-ui/unstyled/generateUtilityClass';

-const avatarClasses = {
-  root: getAvatarUtilityClass('root'),
-  colorDefault: getAvatarUtilityClass('colorDefault'),
-  circular: getAvatarUtilityClass('circular'),
-  rounded: getAvatarUtilityClass('rounded'),
-  square: getAvatarUtilityClass('square'),
-  img: getAvatarUtilityClass('img'),
-  fallback: getAvatarUtilityClass('fallback'),
-};
+const avatarClasses = generateUtilityClass('MuiAvatar', [
+  'root',
+  'colorDefault',
+  'circular',
+  'rounded',
+  'square',
+  'img',
+  'fallback',
+]);

 export default avatarClasses;

This should also open the door to global customization of the class name generator.

[mui-pr-bot]

@material-ui/core: parsed: +0.20% , gzip: +0.20%
@material-ui/lab: parsed: +0.19% , gzip: +0.22%

Details of bundle changes

Generated by 🚫 dangerJS against ab158a4

[mnajdova]

Thanks for taking the time to try this @oliviertassinari. I agree with all points.

Why is getAvatarUtilityClass public?

At some point I needed this in a different component, it may not be true at this moment.. I guess after applying this change we won't even need it anymore

diff --git a/packages/material-ui/src/Avatar/avatarClasses.js b/packages/material-ui/src/Avatar/avatarClasses.js
index 5ede5b894e..2c8a4549da 100644
--- a/packages/material-ui/src/Avatar/avatarClasses.js
+++ b/packages/material-ui/src/Avatar/avatarClasses.js
@@ -1,15 +1,13 @@
-export function getAvatarUtilityClass(name) {
-  return `MuiAvatar-${name}`;
-}
+import generateUtilityClass from '@material-ui/unstyled/generateUtilityClass';

-const avatarClasses = {
-  root: getAvatarUtilityClass('root'),
-  colorDefault: getAvatarUtilityClass('colorDefault'),
-  circular: getAvatarUtilityClass('circular'),
-  rounded: getAvatarUtilityClass('rounded'),
-  square: getAvatarUtilityClass('square'),
-  img: getAvatarUtilityClass('img'),
-  fallback: getAvatarUtilityClass('fallback'),
-};
+const avatarClasses = generateUtilityClass('MuiAvatar', [
+  'root',
+  'colorDefault',
+  'circular',
+  'rounded',
+  'square',
+  'img',
+  'fallback',
+]);

 export default avatarClasses;

[mnajdova]

It took me about 1 hour without knowing exactly what I was doing, I tried to copy the changes in #24107 😆

At least it means that after we have an agreed upon template the migration should go fast and would be pretty mechanical :)

[oliviertassinari]

Why is getAvatarUtilityClass public?

Actually, I have seen it used in Badge.js, but if we have a generateUtilityClass() we might not need it :).

Thanks for taking the time to try this @oliviertassinari. I agree with all points.

Ok, In this case, I think that we could handle them in a follow-up effort.

At least it means that after we have a agreed apon template the migration should go fast and would be pretty mechanical :)

Yeah, I could probably have moved faster if I had done more and had a better template. But the review phase will probably consume it.

I didn't look into the unstyled part yet, but it seems to be important, at the very least, so developers can leverage the components and componentsProps props on the existing components. I also didn't look into updating the demos in the documentation. I feel like splitting the work into multiple chunks like @eps1lon proposed would be better. The timing is another topic. Maybe we could do the documentation update during the beta phase. If adding unstyled components isn't breaking, wish doesn't seem to be, but not sure, I would also suggest we do it during the beta phase. The sooner we can move from alpha to beta the better. Once we have done all the breaking changes, developers will feel a lot more confident upgrading.

[mnajdova]

I didn't look into the unstyled part yet, but it seems to be important, at the very least, so developers can leverage the components and componentsProps props. I also didn't look into updating the demos in the documentation. I feel like splitting the work into multiple chunks like @eps1lon proposed would be better. I'm not sure about the timing, but it's another topic. Maybe we could do the documentation update during the beta phase. If adding unstyled components isn't breaking, wish doesn't seem to be, but not sure, I would also suggest we do it during the beta phase. The sooner we can move from alpha to beta the better. Once we have done all the breaking changes, developers will feel a lot more confident upgrading.

Agree, I think we can push first with the migration to emotion, and proceed with the unstyled versions after that. The documentation can be done anytime, but I agree it's probably better if we push it for the beta version, so that we can release the alpha sooner 👍