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

[IconButton] Custom non-palette color causes TypeError #33054

Closed
2 tasks done
emlai opened this issue Jun 7, 2022 · 39 comments 路 Fixed by #34521
Closed
2 tasks done

[IconButton] Custom non-palette color causes TypeError #33054

emlai opened this issue Jun 7, 2022 · 39 comments 路 Fixed by #34521
Labels
bug 馃悰 Something doesn't work component: icon button This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@emlai
Copy link
Contributor

emlai commented Jun 7, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 馃槸

When I augment IconButton with new colors that don't exist in the palette:

declare module "@mui/material/IconButton" {
  interface IconButtonPropsColorOverrides {
    textPrimary: true;
    textSecondary: true;
  }
}

and then try to use these colors:

<IconButton color="textPrimary">
  <CloseIcon />
</IconButton>

My app crashes with:

TypeError: Cannot read properties of undefined (reading 'main') 

at the following location:

color: (theme.vars || theme).palette[ownerState.color].main,

Expected behavior 馃

IconButton doesn't throw TypeError when using custom color. Instead it handles it safely, like SvgIcon:

(theme.vars || theme).palette?.[ownerState.color]?.main ??

Steps to reproduce 馃暪

No response

Context 馃敠

We intend for color="textPrimary" (or color="text.primary", both are fine) to use the text.primary palette color.

Workaround: pass the color to the icon component instead and augment SvgIconPropsColorOverrides typings:

<IconButton>
  <CloseIcon color="textPrimary" />
</IconButton>

Your environment 馃寧

No response

@emlai emlai added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 7, 2022
@siriwatknp siriwatknp added new feature New feature or request good first issue Great for first contributions. Enable to learn the contribution process. component: icon button This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 7, 2022
@siriwatknp
Copy link
Member

The customization capability is not complete. I marked this as an enhancement.

The fix can use optional chaining like in SvgIcon. However, please note that when you customize the color prop, you are opting out of the theme palette so you have to style by:

createTheme({
  components: {
    MuiIconButton: {
      root:  ({ ownerState, theme }) => ({
        ...ownerState.color === 'textPrimary' && {
          // your style
        }
      })
    }
  }
})

@emlai
Copy link
Contributor Author

emlai commented Jun 7, 2022

The customization capability is not complete.

Ah that explains it.

I worked around it by passing the color to the icon component instead, and augmenting SvgIconPropsColorOverrides:

<IconButton>
  <CloseIcon color="textPrimary" />
</IconButton>

@Shubhamchinda
Copy link
Contributor

Is this issue open to working on?
I'd like to take it. @emlai

@emlai
Copy link
Contributor Author

emlai commented Jun 8, 2022

@Shubhamchinda I don't think it's being worked on, so feel free to take it. Thank you!

@Shubhamchinda
Copy link
Contributor

@Shubhamchinda I don't think it's being worked on, so feel free to take it. Thank you!

Great, thanks. I'll take this.

@geraldaddey
Copy link

geraldaddey commented Jun 21, 2022

@Shubhamchinda @emlai

Any progress? Would love to implement this PR

@Shubhamchinda
Copy link
Contributor

Shubhamchinda commented Jun 23, 2022

@Shubhamchinda @emlai

Any progress? Would love to implement this PR

@geraldaddey Give me a day, and I'll submit a PR.

@shachargiladi
Copy link

Is this issue still open to work on? Would like to work on it

@emlai
Copy link
Contributor Author

emlai commented Jul 24, 2022

@shachargiladi 4 weeks since the last response, so I think it was abandoned. I'd say go ahead!

@hilalsidhic
Copy link

Hi is anyone working one this .I'd like to take it. @emlai

@cyberGHostJs
Copy link

Hi is anyone working one this? I'd like to take it.
@emlai

@rizamoyi
Copy link

rizamoyi commented Sep 8, 2022

Hi anyone still working on this? I'd like to take it. @emlai @cyberGHostJs @hilalsidhic

@kushagra010
Copy link
Contributor

kushagra010 commented Sep 8, 2022

Hey, doesn't seems to be an issue for me 馃 . I was thinking to pick this up, but it didn't reproduce for me.
@emlai

@kushagra010
Copy link
Contributor

My bad, reproducible. @rizamoyi have you started work on it or are you working on it? If not, I can pick this up.

@rizamoyi
Copy link

rizamoyi commented Sep 8, 2022

@kushagra010 I started working on it.

@harsh5902
Copy link

is this issue still on or can take it over and work on it?

@rizamoyi
Copy link

@harsh5902 you can take it over

@harsh5902
Copy link

I have solved the this problem and created a pull request, please do review it.

@paintoshi
Copy link

@harsh5902 Why was the PR closed?

@siriwatknp
Copy link
Member

The PR to fix this issue should:

  • custom color works with TypeScript
  • make sure that the custom color does not crash
  • add a demo to show that the color prop can be augmented

@ZeeshanTamboli if you are available, this could be one to pick up.

@siriwatknp siriwatknp added bug 馃悰 Something doesn't work and removed new feature New feature or request labels Sep 29, 2022
@ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli if you are available, this could be one to pick up.

I am on a bit of a break currently and won't be working next 3-4 weeks much effectively. If somebody else wants to take a look till then, please go ahead.

@the-mgi
Copy link
Contributor

the-mgi commented Sep 29, 2022

this is working fine i guess, i just tested it.
at first you need to provide the props in Palette and PaletteOptions interface that lies in the @mui/material/styles/, the color names that you want to add into the project

declare module '@mui/material/styles' {
  interface Palette {
    primaryVariant500: Palette['primary'];
    secondaryVariant500: Palette['primary'];
  }

  interface PaletteOptions {
    primaryVariant500?: PaletteOptions['primary'];
    secondaryVariant500?: PaletteOptions['primary'];
  }
}

and add these lines for IconButton

declare module "@mui/material/IconButton" {
  interface IconButtonPropsColorOverrides {
    primaryVariant500: true;
    secondaryVariant500: true;
  }
}

these enabling the createTheme to be aware of these colors, then you need to provide the implementation in the theme that would be passed in the themeProvider, like so

export const theme = createTheme({
  palette: {
    primaryVariant500: {
      main: '#ff0000'
    },
    secondaryVariant500: {
      main: '#0000ff'
    },
 }
});

because we do want to let the mui know the values of the color.
hope it helps. thanks.

code-theme
result
tsxCode

@emlai the problem for you might be that you're not writing up the color prop in createTheme for palette

@emlai
Copy link
Contributor Author

emlai commented Sep 29, 2022

@the-mgi Yes, if your palette has the exact same key as the specified color, then this will work fine.

But we're intending to use color="textPrimary" or color="text.primary" to access the usual text.primary.main that we have in our palette. Currently neither of those work.

@the-mgi
Copy link
Contributor

the-mgi commented Oct 1, 2022

@the-mgi Yes, if your palette has the exact same key as the specified color, then this will work fine.

But we're intending to use color="textPrimary" or color="text.primary" to access the usual text.primary.main that we have in our palette. Currently neither of those work.

got it. thanks.

@TCsTheMechanic
Copy link

TCsTheMechanic commented Oct 19, 2022

Hello there, is there any work around ? I'm still having the same problem.

mui.d.ts
Screenshot from 2022-10-19 11-34-06

LightTheme.tsx
Screenshot from 2022-10-19 11-34-49

Any component that I try to change the color
Screenshot from 2022-10-19 11-36-35
Screenshot from 2022-10-19 11-49-04
Screenshot from 2022-10-19 11-53-43

Actually it works with Typography, but I think it's the only one
Screenshot from 2022-10-19 11-38-37

@siriwatknp @the-mgi @kushagra010

@emlai
Copy link
Contributor Author

emlai commented Oct 21, 2022

@TCsTheMechanic Workaround for IconButton is to pass the color prop to the icon component instead: #33054 (comment)

@TCsTheMechanic
Copy link

TCsTheMechanic commented Oct 21, 2022

@emlai also doesn't work

Screenshot from 2022-10-21 09-35-40

@emlai
Copy link
Contributor Author

emlai commented Oct 21, 2022

@TCsTheMechanic Did you augment the typings:

declare module "@mui/material/SvgIcon" {
  interface SvgIconPropsColorOverrides {
    "textColor.light": true;
  }
}

@TCsTheMechanic
Copy link

@emlai still nothing, I'm using @mui/icons-material, I don't know if it follows the same module

@ZeeshanTamboli
Copy link
Member

I am not sure what is the issue here. Custom color does work with typescript with module augmentation and can be augmented. If the custom color is not defined in the palette it will crash.
CodeSandbox: https://codesandbox.io/s/vigorous-bardeen-7pnhv6?file=/src/App.tsx

@emlai
Copy link
Contributor Author

emlai commented Nov 4, 2022

@ZeeshanTamboli

If the custom color is not defined in the palette it will crash.

That is exactly the issue. In our use case the custom color is not in the palette. The same custom color works fine for the icon component. See the original issue description.

@emlai emlai changed the title [IconButton] Custom color causes TypeError [IconButton] Custom non-palette color causes TypeError Nov 4, 2022
@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Nov 5, 2022

@emlai

In our use case the custom color is not in the palette

Then what styles are you applying with the custom color? Why are you calling it with color=textPrimary? What is the use of it without having it in palette?

The same custom color works fine for the icon component.

Yes it works, but if it is not in the palette why should it unnecessarily calculate theme.palette?.color?.main by letting it have nullish check just to prevent crash, but actually not using it?

See the original issue description.

In it you need text.primary palette value, and calling viacolor=text.primary means it's like sx prop syntax and direct sx properties are not supported in IconButton component.

Am I missing something here? Please enlighten me.

@siriwatknp
Copy link
Member

@ZeeshanTamboli The issue is solid. From my understanding, it is intended that the color prop can be customized from the interface IconButtonPropsColorOverrides but developers get TypeError which is not expected.

This is not about the color value (that's the developers' decision), not our. We just provide a way to extend the color.

This PR will fix the issue: #34521

@widersky
Copy link

widersky commented Jan 10, 2023

@siriwatknp Hello, it looks like issue still occurs for me. It's working perfectly with Card, SvgIcon and Typography components, but not for Button and IconButton - I have error:

Uncaught TypeError: Cannot read properties of undefined (reading 'type')

(colorManipulator.js:50)

I use latest libs versions:

"@mui/icons-material": "^5.11.0",
"@mui/material": "^5.11.4",
"typescript": "^4.9.4",

I tried to reinstall dependencies by removing node_modules and yarn.lock but it didn't help.

@siriwatknp
Copy link
Member

@siriwatknp Hello, it looks like issue still occurs for me. It's working perfectly with Card, SvgIcon and Typography components, but not for Button and IconButton - I have error:


Uncaught TypeError: Cannot read properties of undefined (reading 'type')

(colorManipulator.js:50)

I use latest libs versions:


"@mui/icons-material": "^5.11.0",

"@mui/material": "^5.11.4",

"typescript": "^4.9.4",

I tried to reinstall dependencies by removing node_modules and yarn.lock but it didn't help.

Pleasr share a Code sandbox that reproduces the issue.

@Charul-Parspec
Copy link

@siriwatknp Facing same issue for icons imported from '@icons-material'. module augmentation is not working for @mui/icons-material. @TCsTheMechanic did you resolve the issue ?

@nikolay-bennie
Copy link

why is the issue closed if the issue is reproducible?

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Mar 22, 2024

@nikolay-bennie Please provide a reproduction with the latest Material UI version.

@erwin-huang
Copy link

Hi @siriwatknp @ZeeshanTamboli

I also got this issue

Uncaught TypeError: Cannot read properties of undefined (reading 'type')

To reproduce:

  1. Create a custom palette color, customColor and assign a random color.
  2. Create a custom IconButton color, customColor, and assign random styles.

Note: The "custom palette color name" should be the same as the "custom IconButton color name".

Here is the code sandbox, try to access /issue route to check the issue.
https://codesandbox.io/p/devbox/nextjs-mui-icon-button-mmml75

I think I did the config correctly, didn't I?

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: icon button This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.