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

[Tab] SVG icon takes color from Icon's color props as well.. #7107

Closed
wants to merge 5 commits into from

Conversation

RejinR
Copy link

@RejinR RejinR commented Jun 12, 2017

This is an improvement on the pull request #7091..
The color can now be passed from the color prop also instead of just style.

@oliviertassinari
Copy link
Member

I'm not sure to follow, what's the use case for that color property? I don't think it fit into the whole Material-UI API.

@RejinR
Copy link
Author

RejinR commented Jun 12, 2017

Developers may not want the default white color to come by default to the icon.. So adding an option to change the color can help in customising their tab component to their liking if necessary..
So now we can mention the color prop by either including it in the style or by using the color prop that is defined for the Icon component..

iconProps.color = icon.props.style.color;
} else {
iconProps.color = styles.root.color;
if (icon.props) {
Copy link
Member

Choose a reason for hiding this comment

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

This all logic is getting too much complex. I think that the correct fix is the following:

// If it's svg icon set color via props
if (icon.type.muiName === 'SvgIcon') {
  iconProps.color = icon.props.color || styles.root.color;
}

Copy link
Member

Choose a reason for hiding this comment

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

It would be perfect if you could add some unit test to ensure it.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module! PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jun 12, 2017
@oliviertassinari
Copy link
Member

I have figured out how to simplify this in #7131. Let me know what you think about it.

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: tabs This is the name of the generic UI component, not the React module! PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants