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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[system] Passing incompatible grey color to palette does not throw #26651

Open
2 tasks done
alexplumb opened this issue Jun 8, 2021 · 10 comments
Open
2 tasks done

[system] Passing incompatible grey color to palette does not throw #26651

alexplumb opened this issue Jun 8, 2021 · 10 comments
Labels
new feature New feature or request package: system Specific to @mui/system

Comments

@alexplumb
Copy link

alexplumb commented Jun 8, 2021

This issue exists in alpha.36

  • 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 馃槸

When creating a custom Chip variant+color combination, I see an error in the JavaScript console that throws an exception. Note that custom variant props work, but not color.

Expected Behavior 馃

I should be able to customize both the variant and colour of a Chip.

Steps to Reproduce 馃暪

https://codesandbox.io/s/mui-chip-error-1vntn?file=/src/index.js

Steps:

Context 馃敠

I started receiving this error message after upgrading from alpha.24 to alpha.36

Your Environment 馃寧

`npx @material-ui/envinfo`
  Don't forget to mention which browser you used.
  Output from `npx @material-ui/envinfo` goes here.
@alexplumb alexplumb added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 8, 2021
@eps1lon
Copy link
Member

eps1lon commented Jun 9, 2021

Thanks for the report.

The only failing combination is <Chip color="grey" label="filled grey" variant="filled" />.

The reason being that the shape of the grey color is invalid. It should match

export interface Color {
  50: string;
  100: string;
  200: string;
  300: string;
  400: string;
  500: string;
  600: string;
  700: string;
  800: string;
  900: string;
  A100: string;
  A200: string;
  A400: string;
  A700: string;
}

while you probably assumed

export interface PaletteColor {
  light: string;
  main: string;
  dark: string;
  contrastText: string;
}

in https://codesandbox.io/s/mui-chip-error-1vntn?file=/src/theme.js:650-773

This seems like an easy to make mistake that we should catch earlier at runtime and throw during createTheme.

@eps1lon eps1lon added new feature New feature or request package: system Specific to @mui/system and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 9, 2021
@eps1lon eps1lon changed the title [Chip] "Cannot read property 'type' of undefined" error when using custom colours [system] Passing incompatible grey color to palette does not throww Jun 9, 2021
@eps1lon eps1lon changed the title [system] Passing incompatible grey color to palette does not throww [system] Passing incompatible grey color to palette does not throw Jun 9, 2021
@mnajdova
Copy link
Member

mnajdova commented Jun 9, 2021

This seems like an easy to make mistake that we should catch earlier at runtime and throw during createTheme.

Agree, the typings are correct, but for this specific color I believe we can add runtime check as well 馃憤

@eps1lon
Copy link
Member

eps1lon commented Jun 10, 2021

Agree, the typings are correct, but for this specific color I believe we can add runtime check as well +1

Ideally the shape would be consistent to begin with considering the Color interface (the one with the A100 etc swatches) is something specific to Material design as far as I can tell.

@alexplumb
Copy link
Author

Thanks for the additional information - I wasn't aware that grey was special like this. Any idea why it started failing between alpha.24 and alpha.36?

@alexplumb
Copy link
Author

I don't really understand how to fix this. Even if I rename the colour something different - e.g. "greyscale" instead of "grey" - I still get the same error.

I just can't get this one particular chip to render.

Here's a sandbox link: https://codesandbox.io/s/mui-chip-error-forked-10qsb?file=/src/theme.js

@mnajdova
Copy link
Member

@alexplumb could you provide a codesandbox with minimal theme that causes the break? The theme in the sandbox linked is 300+ lines. It would help if you could isolate only the bits which are important for reproducing the issue.

@alexplumb
Copy link
Author

@mnajdova I've updated the sandbox in my previous comment to be as minimal as possible: https://codesandbox.io/s/mui-chip-error-forked-10qsb

@mnajdova
Copy link
Member

mnajdova commented Jul 1, 2021

@alexplumb it was easier to see what was wrong now :) You were missing contrastText for the greyscale. Here is a working codesandbox - https://codesandbox.io/s/mui-chip-error-forked-4ktgb?file=/src/theme.js

@alexplumb
Copy link
Author

Fascinating - thanks so much! This change actually makes the original code sandbox work as well. So strange - should the issue title be changed then? I'll leave that to you folks :)

@mnajdova
Copy link
Member

mnajdova commented Jul 1, 2021

We could keep the issue as a reminder to add a run time check for incompatible grey input in the theme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: system Specific to @mui/system
Projects
None yet
Development

No branches or pull requests

3 participants