search

nuxt / icon

The <Icon> component, supporting Iconify, Emojis and custom components.
https://stackblitz.com/edit/nuxt-icon-playground?file=app.vue
MIT License
949 stars 57 forks source link

[Bug] Do not strip custom local icons of their classes #210

Open stefanobartoletti opened 3 months ago

stefanobartoletti commented 3 months ago

I noticed a behavior that I consider a bug, but I'm not sure if this is wanted.

Anyway, I noticed that when using custom local icons, i.e. loaded from the assets folder, classes applied to their root svg element are stripped when the icon is actually loaded in the template.

This is not really a good behavior, because some classes could be used to apply animations to the said icon, and removing them will cause them to not be animated.

I prepared a simple reproduction here https://github.com/stefanobartoletti/icon-test, everything should be pretty much self-explanatory, but right now, the SVG icon directly pasted into the template can be animated via the custom icon-animation class, while the same SVG loaded with the Icon component cannot, because it is stripped of the said class.

A possible workaround right now could be to give this class to the Icon component, instead of the source SVG icon, but this is not optimal, since the same exact icon could be needed in many places in a project and it would make sense to give this class only once to the source, instead of needing to apply it to every and each instance of the Icon component using it.

Also, I noticed that the classes are stripped only from the root svg element, if you have classes in nested g or path elements, they remain in place.

Hopefully it is all clear, anyway, feel free to ask for more info if needed.

Edit:

I forgot to add that I'm talking about a configuration using the svg mode to load icons, where the source icon is inlined in the template.

antfu commented 3 months ago

At this line:

https://github.com/nuxt/icon/blob/1b28ec2a0e01d39229ded06c4584f984f428caab/src/module.ts#L359

While parseSVGContent correctly parsed svg data with the root attributes, convertParsedSVG discarded them. At the interface of IconifyIcon, it doesn't have a field to represent custom attributes. This downs to Iconify does not support root attributes for icons in the JSON data format.

/cc @cyberalien is that something Iconify would consider in scope? If so I'd happy to send PR upstream to get this supported

userquin commented 3 months ago

@antfu parseSVGContent will return the attributes, nuxt icon ignoring them in your link: https://github.com/iconify/iconify/blob/main/packages/utils/src/svg/parse.ts#L42-L45

EDIT: convertParsedSVG ignoring custom attributes : that's weird, attributes should be applied in the build function

cyberalien commented 3 months ago

This is a tricky issue.

Current implementation is by design. IconifyIcon type is not designed to handle additional attributes, attributes are intended to be removed from SVG element.

Why is it so?

Editors like Adobe Illustrator (the worst of them, closely followed by Inkscape) add massive amounts of garbage to SVG it generates, all of it is always unused. The most common example of such garbage on SVG tag exported by various editors is id attribute. It is never used, value is a layer name.

Not having additional attributes makes it easy to deal with icon when rendering it. All that is added to SVG tag is viewBox attribute. Attributes like stroke-width (which was first similar issue I've encountered with valid use case a while ago) that some icons include are always moved to child elements when icon is optimised, but for CSS stuff applied to icon this solution won't work.

Should it be changed?

For this use case it makes sense to keep attributes.

Custom loaders should not modify user's icon. In this case it is absolutely safe. In cases when user provides a really bad icon with lots of Illustrator stuff without optimising it, it should still be kept as is and should be up to user to make sure icon is correct.

This was not considered many years ago when type was created, but I think it is about time to change it.

cyberalien commented 3 months ago

Need to think of a temporary solution for this issue.

Proper solution would require changing type, but simply adding extra attributes to type is not a good idea because I'd rather completely rebuild all types.

What I want to do with IconifyIcon type:

Additionally, for IconifyJSON type I want to simplify icons, getting rid of flip/rotation (it uses IconifyIcon type) and simplifying aliases (value as string, not object, so everything is simple to use).

However, this is a huge task because first need to finalise types to make sure everything is included, then change tools to handle both new and old types, then change all components to handle new type, only then actually use new type.

So it won't work for this issue because it will take a lot of time.

stefanobartoletti commented 3 months ago

@cyberalien I'm not sure if this is a valid suggestion, but the only needed attribute here is the class one.

Would this pipeline work?

cyberalien commented 3 months ago

Unfortunately I don't think it would. Can't figure out how to make it work. Icons are converted to icon set in IconifyJSON format, which does not support extra attributes. Even if extra attributes are added, it would require changing unrelated @iconify/vue package to handle it.

I think best option for now is to change your icons. You know names of icons, which have those custom class names. When using those icons in code, you can add necessary class names to icon component.

Proper support can be added later with types redesign.