[SvgIcon] Remove component prop #17287
Conversation
eps1lon
added
breaking change
Details of bundle changes.Comparing: 7c3be01...37e1344
|
|
What do we do with this demo https://material-ui.com/components/icons/#svg-icons? |
|
I believe that the |
Yep already fixed that but forgot to push. This is why I said the new approach is even better IMO. |
I wish it could be that simple, but I think we need to add another constraint. Most people are using the pre-maid SVG icons. The solution was designed to accept |
|
What percentage of the user-base do you estimate is performing advanced customisation of the stock icons in a way that required the component prop? (Or, perhaps, how many fingers of one hand does it represent?! 😉) We should balance making it marginally easier for the few, vs, the overhead for everyone else. I'm in favour of this change. Those that need to go further can do as you have done in this demo and create the icon as needed using the SvgIcon component (it's what it's there for after all). |
|
Removing it and seeing nobody complain would be a good confirmation of the hypotheses. |
Could you add the code that was using the |
|
I don't understand your question. To be more clear. What happens if we keep the same customization demo but we replace the custom home icon by the ready made one from @material-ui/icons? |
Could you add the code that uses this customization option? It's not clear to me why we need a The documented customization can be solved with less and clearer code. |
|
This one import React from 'react';
import { makeStyles } from '@material-ui/core/styles';
import { blue, red } from '@material-ui/core/colors';
import SvgIcon from '@material-ui/core/SvgIcon';
import HomeIcon from '@material-ui/icons/Home';
const useStyles = makeStyles(theme => ({
root: {
'& > svg': {
margin: theme.spacing(2),
},
},
iconHover: {
'&:hover': {
color: red[800],
},
},
}));
/*
function HomeIcon(props) {
return (
<SvgIcon {...props}>
<path d="M10 20v-6h4v6h5v-8h3L12 3 2 12h3v8z" />
</SvgIcon>
);
}
*/
export default function SvgIcons() {
const classes = useStyles();
return (
<div className={classes.root}>
<HomeIcon />
<HomeIcon color="primary" />
<HomeIcon color="secondary" />
<HomeIcon color="action" />
<HomeIcon className={classes.iconHover} color="error" style={{ fontSize: 30 }} />
<HomeIcon color="disabled" fontSize="large" />
<HomeIcon
color="primary"
fontSize="large"
component={svgProps => {
return (
<svg {...svgProps}>
<defs>
<linearGradient id="gradient1">
<stop offset="30%" stopColor={blue[400]} />
<stop offset="70%" stopColor={red[400]} />
</linearGradient>
</defs>
{React.cloneElement(svgProps.children[0], {
fill: 'url(#gradient1)',
})}
</svg>
);
}}
/>
</div>
);
} |
|
Thats changed in this PR. |
|
The SVG path has to be copy and pasted in the developer codebase. We are giving up on supporting the icons package. To be honest, it sounds OK to remove the prop, but maybe some of our users have found a legitimate use case. I really think that we should start with a deprecation, something we can merge in a few months, then continue with removal if nobody complains. What do you think? |
|
I think that we should keep this prop. I have removed the gradient demo from the documentation, as likely covering a too narrow use case. The import svg files use case is interesting. |
Why? Especially with the more complicated customization example removed there isn't even a documented use case anymore |
|
I think that it's important so people can:
|
4 participants
componentprop makes a component more complicated. We do have it documented but I can't see how this makes the component easier to customize. I would even argue that the change improved customization.In any case this change is queued for v5