Skip to content
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

Closed
wants to merge 9 commits into from
Closed

Inline symbolset option #430

wants to merge 9 commits into from

Conversation

Dietr
Copy link
Contributor

@Dietr Dietr commented Sep 14, 2023

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

  • added an component which contains the svg symbolset.
  • if you place the in your main template the component will reference the id of the symbol inline. So no external files are needed.
  • you can still use the external svg if you don't use the component.

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?

@Dietr Dietr added the enhancement Used when the PR adds a new feature or enhancement. label Sep 14, 2023
@Dietr Dietr requested review from Windvis and elpoelma September 14, 2023 08:42
Copy link
Collaborator

@elpoelma elpoelma left a 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.

@Windvis
Copy link
Contributor

Windvis commented Sep 14, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

Dietr commented Sep 15, 2023

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.

@Dietr Dietr marked this pull request as draft September 15, 2023 09:47
@Windvis
Copy link
Contributor

Windvis commented Mar 20, 2024

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

@Windvis Windvis closed this Mar 20, 2024
@Windvis Windvis deleted the feature/inline-symbolset branch July 10, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Used when the PR adds a new feature or enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants