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

[IconButton] Fix default color prop #26064

Merged
merged 3 commits into from
Apr 30, 2021
Merged

[IconButton] Fix default color prop #26064

merged 3 commits into from
Apr 30, 2021

Conversation

Jack-Works
Copy link
Contributor

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 30, 2021

No bundle size changes

Generated by 🚫 dangerJS against 3944919

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank! Could you also adjust the .propTypes in IconButton.js so that the documentation is updated as well?

@oliviertassinari oliviertassinari added the component: icon button This is the name of the generic UI component, not the React module! label Apr 30, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a leftover from #21594. We still want to kill 'default' as a valid name: #13028. In SvgIcon/Icon components, this default color is called action. Would this name work?

@oliviertassinari oliviertassinari changed the title fix(IconButton): add missing default color in the type [IconButton] Fix default color prop Apr 30, 2021
@Jack-Works
Copy link
Contributor Author

It looks like a leftover from #21594. We still want to kill 'default' as a valid name: #13028. In SvgIcon/Icon components, this default color is called action. Would this name work?

I see "default" still getting processed in the JS file. So you mean I should replace 'default' by 'action'?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 30, 2021

@Jack-Works I think that there are two layers to this problem:

  1. Fix the types/prop-types to match the implementation, this is what you started and Sebastian proposed to push further. This is already a win on its own, it could be merged as-is.
  2. Fix the name of the default color. You have understood my proposal correctly. I think that it could be done here, or later.

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Apr 30, 2021
@oliviertassinari oliviertassinari dismissed their stale review April 30, 2021 18:01

I guess we can start slow, a fix first

@oliviertassinari oliviertassinari merged commit 9d66ee0 into mui:next Apr 30, 2021
@oliviertassinari
Copy link
Member

@Jack-Works Thanks for raising

@Jack-Works
Copy link
Contributor Author

image

Upgraded to alpha 33 but I see there is no "action" type

@oliviertassinari
Copy link
Member

@Jack-Works Yes, the idea with this iteration was to fix the types, to have them sound. Regarding renaming default -> action, it's not something that we have done yet, this is for #13028.

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants