Conversation
|
|
||
| /** | ||
| * @since 1.0 | ||
| * @since 5.2 |
There was a problem hiding this comment.
THe since indicates since when the component has been available. Pleas revert
| * | ||
| * Icon library for this instance. Overrides the global default. | ||
| * - "material-icons" → Material Icons | ||
| * - "material-icons" → Material Icons |
| }) | ||
| 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.
| if (this.src !== undefined) { | ||
| if (isSvg) { | ||
| if (this.inlineSvg) { | ||
| return ( | ||
| <svg part="svg"> | ||
| <use href={`${this.src}#img`} /> | ||
| </svg> | ||
| ); | ||
| } | ||
| return <img src={this.src} />; | ||
| } | ||
| return this.src; | ||
| } | ||
| return <slot />; |
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..
| }; | ||
|
|
||
| private renderContent(isSvg: boolean) { | ||
| if (this.src !== undefined) { |
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.
| <six-icon>sick</six-icon> | ||
| <six-icon>fort</six-icon> | ||
| <six-icon>castle</six-icon> | ||
| <six-icon src="fort"></six-icon> |
There was a problem hiding this comment.
I would add a comment explaining the two possible ways to do this => slot and prop
| } | ||
|
|
||
| /* ========================================================================== | ||
| Size helpers (unchanged API, a bit more explicit) |
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: