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

[Container] classes generated wrongly after migration to emotion #24280

Closed
2 tasks done
povilass opened this issue Jan 5, 2021 · 8 comments
Closed
2 tasks done

[Container] classes generated wrongly after migration to emotion #24280

povilass opened this issue Jan 5, 2021 · 8 comments
Assignees
Labels
bug 🐛 Something doesn't work component: Container The React component

Comments

@povilass
Copy link
Contributor

povilass commented Jan 5, 2021

  1. Something interesting happening to clsx if classes.disableGutters is undefined or classes.fixed is undefined

image
image

But both combination produces just first undefined?
image

  1. While using maxWitdh=false generates unnecessary class MuiContainer-maxWidthFalse
    image
  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

  1. generates class MuiContainer-maxWidthFalse
  2. Generates undefined classes on fixed, disableGutters properties while using clsx.

Expected Behavior 🤔

  1. Using maxWitdh=false should not generate class maxWidthFalse
  2. No undefined classes should be applied.

Steps to Reproduce 🕹

Updated version from "@material-ui/core": "5.0.0-alpha.21" to "@material-ui/core": "5.0.0-alpha.22"
Reference playground

Overall this probably should be changed to this.

const useUtilityClasses = (styleProps) => {
  const { classes = {}, fixed, disableGutters, maxWidth } = styleProps;

  const maxWidthClassName = `maxWidth${capitalize(String(maxWidth))}`;

  return {
    root: clsx(
      containerClasses.root,
      classes.root,
      typeof maxWidth === "string" && getContainerUtilityClass(maxWidthClassName),
      typeof maxWidth === "string" && classes[maxWidthClassName],
      fixed && containerClasses.fixed,
      fixed && classes.fixed,
      disableGutters && containerClasses.disableGutters,
      disableGutters && classes.disableGutters,
    )
  };
};

Context 🔦

Updating version.

@povilass povilass added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 5, 2021
@povilass
Copy link
Contributor Author

povilass commented Jan 5, 2021

@oliviertassinari has a proposal solution in #24250 (comment)

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: Container The React component and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 5, 2021
@mnajdova
Copy link
Member

@povilass can you please share a repro where you see the undefined classes? I debugged locally the examples for the container and there aren't any undefined classes, although I don't have any overrides on the classes.

Regarding the maxWidthFalse I don't have a strong reference, but I agree, I think it makes sense not to add it when the value of the prop is false, as it would be more consistent with the other components 👍

@mnajdova
Copy link
Member

Anyway, I agree that we should soon rather then later update the classes generation to something like it is proposed in #24250 (comment) for making sure we keep this behavior consistent across different components.

@povilass
Copy link
Contributor Author

@mnajdova the same code just needed to uncomment old useUtilityClasses code

@mnajdova
Copy link
Member

Should have been fixed by #24371 I believe.

@povilass
Copy link
Contributor Author

OK let's wait for the new version for now I think we can close it.

@oliviertassinari
Copy link
Member

@povilass
Copy link
Contributor Author

povilass commented Jan 12, 2021

@oliviertassinari Yes it works updated and I used https://ci.codesandbox.io/status/mui-org/material-ui/pr/24371 to find the correct pull request and how to install it to my repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Container The React component
Projects
None yet
Development

No branches or pull requests

3 participants