-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inline symbolset option #430
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a pretty good solution to me. It avoids duplication of icon svgs.
Just wondering if the AuSymbols::Content
component should be added to gitignore.
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 |
Ok, thanks for the feedback. Investigated the ember-svg jar and I think we can easily switch behaviour through the strategy. 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. |
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?