appuniversum / ember-appuniversum

Ember addon wrapping the appuniversum components.
https://appuniversum.github.io/ember-appuniversum
MIT License
14 stars 11 forks source link

Inline symbolset option #430

Closed Dietr closed 5 months ago

Dietr commented 1 year ago

Exploration of an idea to improve the current svg symbolset implementation:

I hope this solves the GN editor usecase? I prefer the use of a symbolset as it improves performance and avoids repeating svg code. Inlining the svg should make this solution compatible with most usecases.

Any thoughts on this?

Windvis commented 1 year ago

I'm not the biggest fan of this setup, to be honest. It has some (,in my opinion, serious) technical downsides.

The generated symbol-set is about 150Kb large. Since it is now included in a template it will get compiled and bundled with the js code. So that means the bundle is 150Kb (50kb zipped) larger as well, (even if the component isn't used, when doing classic builds).

I haven't benchmarked, but rendering that component will probably also be quite slow, but this might not be an issue since it will probably only be done once, somewhere in the app template. Nevertheless, it's extra work that wasn't needed before.

I think we should just go the ember-svg-jar route. That would allow apps to add extra icons as well. Which we can't do at the moment.

I don't think it's high prio either since there is already a workaround available?

elpoelma commented 1 year ago

I'm not the biggest fan of this setup, to be honest. It has some (,in my opinion, serious) technical downsides.

The generated symbol-set is about 150Kb large. Since it is now included in a template it will get compiled and bundled with the js code. So that means the bundle is 150Kb (50kb zipped) larger as well, (even if the component isn't used, when doing classic builds).

I haven't benchmarked, but rendering that component will probably also be quite slow, but this might not be an issue since it will probably only be done once, somewhere in the app template. Nevertheless, it's extra work that wasn't needed before.

I think we should just go the ember-svg-jar route. That would allow apps to add extra icons as well. Which we can't do at the moment.

I don't think it's high prio either since there is already a workaround available?

It is not really high prio for the moment as there is a workaround for GN available now. I think this approach (with the inline symbolset) is only useful if your template contains a lot (of the same) icons. If templates contain few icons, I agree that ember-svg-jar seems to be the better route.

Dietr commented 1 year ago

Ok, thanks for the feedback.

Investigated the ember-svg jar and I think we can easily switch behaviour through the strategy. In the 'symbol' strategy it inlines the svg symbols and also compresses it.

For now the default strategy is set to 'inline' as I can't get the {{content-for}} to work in storybook. (any ideas?)

Also rewrote the icon css so it works with a wrapper span.

Windvis commented 5 months ago

Closing since we went for an alternative solution in #483 and #481