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

[SvgIcon] color prop do not recognize "success" and "warning" #26644

Closed
2 tasks
mwskwong opened this issue Jun 8, 2021 · 6 comments
Closed
2 tasks

[SvgIcon] color prop do not recognize "success" and "warning" #26644

mwskwong opened this issue Jun 8, 2021 · 6 comments
Labels
component: SvgIcon The React component. status: waiting for author Issue with insufficient information

Comments

@mwskwong
Copy link
Contributor

mwskwong commented Jun 8, 2021

  • 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 😯

The title is pretty self-explanatory. The SvgIcon component uses the default color when setting color="success" and color="warning" (I expect the same applies to info and custom color palette as well).

Expected Behavior 🤔

It should turn into the corresponding color (typically green and yellow respectively).

Steps to Reproduce 🕹

Steps:
Use any icons from @material-ui/icon@next or SvgIcon from @material-ui/core@next

Context 🔦

I can't think of any reasons why error would work on SvgIcon while warning and success do not.

Your Environment 🌎

`npx @material-ui/envinfo`
  Don't forget to mention which browser you used.
  Output from `npx @material-ui/envinfo` goes here.

  System:
    OS: Windows 10 10.0.18363
  Binaries:
    Node: 14.17.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.5 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 7.14.0 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 91.0.4472.77
    Edge: Spartan (44.18362.449.0)
  npmPackages:
    @emotion/react: ^11.4.0 => 11.4.0
    @emotion/styled: ^11.3.0 => 11.3.0
    @material-ui/core: ^5.0.0-alpha.35 => 5.0.0-alpha.35
    @material-ui/data-grid: ^4.0.0-alpha.30 => 4.0.0-alpha.30
    @material-ui/icons: ^5.0.0-alpha.35 => 5.0.0-alpha.35
    @material-ui/private-theming:  5.0.0-alpha.35
    @material-ui/styled-engine:  5.0.0-alpha.34
    @material-ui/styles: ^4.11.4 => 4.11.4
    @material-ui/system:  5.0.0-alpha.35
    @material-ui/types:  6.0.1
    @material-ui/unstyled:  5.0.0-alpha.35
    @material-ui/utils:  5.0.0-alpha.35
    @types/react:  17.0.9
    react: ^17.0.2 => 17.0.2
    react-dom: ^17.0.2 => 17.0.2
@mwskwong mwskwong 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 8, 2021

It doesn't look like this bug report has enough info for one of us to reproduce it.

Please provide a CodeSandbox (https://material-ui.com/r/issue-template-next), a link to a repository on GitHub, or provide a minimal code example that reproduces the problem.

Here are some tips for providing a minimal example: https://stackoverflow.com/help/mcve

@eps1lon eps1lon added component: SvgIcon The React component. status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 8, 2021
@mwskwong
Copy link
Contributor Author

mwskwong commented Jun 9, 2021

It doesn't look like this bug report has enough info for one of us to reproduce it.

Please provide a CodeSandbox (https://material-ui.com/r/issue-template-next), a link to a repository on GitHub, or provide a minimal code example that reproduces the problem.

Here are some tips for providing a minimal example: https://stackoverflow.com/help/mcve

I thought this issue is obvious enough so I didn't provide a code sandbox link. Anyway here it is:
Edit sparkling-bash-c4sok

@eps1lon
Copy link
Member

eps1lon commented Jun 9, 2021

Thanks for the repro.

<Search color="success" />
<Search color="warning" />
<Search color="info" />

These colors are not supported by default. Hence the TypeScript error. Why do you think these colors are supported by default?

You probably want to add these colors to begin with: https://next--material-ui.netlify.app/customization/palette/#adding-new-colors

I thought this issue is obvious enough so I didn't provide a code sandbox link.

We have 10-20 issues a day. Everytime an "obvious" issue is opened we still need to spend 5 minutes to reproduce it. That's 50-100 minutes a day on "obvious" issues. Now some of these aren't obvious and we need to be careful not to spend more time.

This is time that can be saved for maintainers which we explicitly ask for in the issue template. Generally, if a maintainer asks something, following that does help us more than making educated guesses. Even if these are reasonable.

@mwskwong
Copy link
Contributor Author

mwskwong commented Jun 9, 2021

What do you mean? Those colors are standard in the theme created using createTheme. They have been there since v4 (not sure about v3 and v0.x)
Taken from https://next.material-ui.com/
image

That's why I'm confused. Why the prop validation allows error while rejects other status/levels?

@eps1lon
Copy link
Member

eps1lon commented Jun 9, 2021

Those colors are standard in the theme created using createTheme.

That doesn't mean they're available on every component.

Why the prop validation allows error while rejects other status/levels?

We no longer verify valid color tokens since these might be customized. We rely on static type-checking via TypeScript.

@eps1lon
Copy link
Member

eps1lon commented Jun 9, 2021

Closing in favor of #26192 which should cover this issue as well.

@eps1lon eps1lon closed this as completed Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: SvgIcon The React component. status: waiting for author Issue with insufficient information
Projects
None yet
Development

No branches or pull requests

2 participants