Conversation
There was a problem hiding this comment.
THe since indicates since when the component has been available. Pleas revert
| }) | ||
| export class SixIcon { | ||
| /** Name of the icon, path to SVG file or a data image */ | ||
| @Prop() src?: string; |
There was a problem hiding this comment.
As I already mentioned in #425 (comment)
Using src as the prop name for the input, in case when we are passing an icon name, makes no sense to me. I would separate the different types, meaning
src: SVG File or Data URLname: Name of the icon => you can introduce this already, but keep the slot
If I put myself in the user's shoes, I find this to be a lot easier to understand.
You can then define a hierarchy, like src > name > slot
There was a problem hiding this comment.
Well, i think it makes more sense than having prop name and prop src... how the users know which one takes priority? ... Just makes things more complicated than they should be. I will change it anyway.
| * - <svg><use/></svg> is better for styling (e.g. fill: red), but slower at rendering. | ||
| * - <img> is better for HTTP caching, but you cannot style the internal SVG elements. | ||
| */ | ||
| @Prop({ reflect: true }) inlineSvg: boolean = false; |
There was a problem hiding this comment.
Is this really needed? What performance hit are talking about?
I would simply default to the <svg><use/></svg> since, as you mention, its better for styling
There was a problem hiding this comment.
As you can see there img is good for HTTP caching... you can in some instances visually see the difference rendering/loading the icons...
There was a problem hiding this comment.
There is however a caveat which you have to put id="img" inside your svg for it to work and properties inside svg have precedence i.e. if you have fill in some vector you cannot override it from outside.
There was a problem hiding this comment.
I am not sure if there is a more elegant way to approach this... this is what i have at the moment.
There was a problem hiding this comment.
if (this.src === undefined) {
return <slot />;
}
if (!isSvg) {
return this.src;
}
if (!this.inlineSvg) {
return <img src={this.src} />;
}
return (
<svg part="svg">
<use href={`${this.src}#img`} />
</svg>
);
There was a problem hiding this comment.
Your solution does not work if you want to have name prop... i will adjust it..
There was a problem hiding this comment.
So the svg here has precedence over the slot?
There was a problem hiding this comment.
Why not? When you explitly place a path to a SVG you are showing a more clear intention to use svg over anything else.
There was a problem hiding this comment.
I would add a comment explaining the two possible ways to do this => slot and prop
There was a problem hiding this comment.
Remove the content inside the brackets
3 participants
🔗 Linked issue
❓ Type of change
📚 Description
#425
📝 Checklist
mainbranchfix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information: