Add <FaStaticSprite> placeholder to transform icons #138
Conversation
This will move sprite files from any installed fontawesome builds into the public directory so they're available for use.
This placeholder value will now be transformed into a static html svg element that references the correct sprite.
Each icon has default width and height information applied through classes. Adding them to the sprite ensures that sprite icons and component icons are visually interchangeable.
|
This is going in the correct direction in my opinion. I like that it's explicit. Both for developer and compiler it's clear that this most be transformed to a static DOM element at build time. This seems to be also inline with the overall direction the ember ecosystem is going. E.g. the macro system proposed by Embroider RFC also adds a Some open questions from my side:
|
Please don't. This currently is the most annoying thing about this addon: Having to manually list all the icons you use. And if someone isn't careful he/she forgets to remove the icon from the list, which just got deleted in all other places. If people really want to write things twice and manually parse the app to figure out what icons need to be listed, please make it configurable, so everybody else can let the machines do that kind of work. |
| @@ -48,8 +48,10 @@ | |||
| ] | |||
| }, | |||
| "dependencies": { | |||
| "@fortawesome/fontawesome-free": "^5.12.1", | |||
There was a problem hiding this comment.
Yes, it's where the sprites seem to live, are they somewhere else? I figured a zero install experience for free icons was better than documenting a separate install step.
There was a problem hiding this comment.
Nope, they live there and that's the correct place to get them from. But this would force people who are just using Pro to download the extra package. I also worry about someone who is trying to use an older version for one reason or another (we have those folks who aren't ready to upgrade) This is going to add a competing version of the package that might have to contend with hoisting.
There was a problem hiding this comment.
Makes sense, I'll replace with installation instructions.
|
I have some thoughts:
This is a great idea. We've had more and more interest in rendering the icons at build time (Next.js, Nuxt.js, etc.) so this is a good direction in my opinion. |
|
Thanks everyone - great questions and information.
I tested with 5K icons in the dummy app and it added less than one second to the total build time. With no icons included in templates it doesn't change build time in a measurable way.
Yes, currently the bundle size grows if there are more icons. It's better than the current solutions because the link to the sprite is not as verbose as the SVG itself. I think this is likely a bug in my implementation though, so I'm hoping for some help in discord to solve it.
Right now all icon sprite files are being included magically, but they are placed into
I'd like to test this in the wild before recommending a change, but I do think that for purely static invocations this may be the better path for most usage. The API for things like spin and transform might be more difficult though so it might not be the right choice every time.
This is one thing I really waffled on. I'm ok with either.
Yes, the entire sprite loads the first time it is used. I'm not thrilled with this, but I haven't yet figured out how to detect icon usage in time to modify the build because of the way the ember build works. I think if we get that ability this would be a great enhancement.
Thanks for the reminder, I meant to add this to the docs. I think a warning in the docs should be sufficient and allow application owners to decide how to proceed. |
If you have anything to try I will be happy to give it a spin. I just tried to instrument our app via |
|
Did some performance test : 351 ms with the sprite instead of 1.46s for 208 icons in each component |
|
https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href => xlink:href will be deprecated in svg 2 |
| const title = this.builders.text(mappedAttributes['@title'].chars); | ||
| children.push(this.builders.element('title', null, null, [title])); | ||
| } | ||
| const xlink = this.builders.attr('xlink:href', this.builders.text(`${spritePath}#${iconName}`)); |
There was a problem hiding this comment.
without the leading / on ${spritePath} , it is not looking on root url /assets but /user/assets for example
|
css styles do not apply to svg => color become fill to changes icon color |
|
If I use your branch in package.json: And add this icon: I see this error: I have no idea why I am seeing this... adding |
|
Thanks @st-h, I heard that the glimmer API changed substantially in 3.17. It's one of my hesitations about adding this type of build time transform since that API isn't under semver. I'll investigate. |
|
OK, I have some performance numbers for discussion. (thanks @st-h for digging up the right way to do this).
Then I created a bunch of routes to render 1K, and 10K icons by placing one on each line: of in a loop as: First of all a note on file size: Adding 10K build time transformed coffee icons results in a 3MB (18kb gzipped) of additional javascript over a component or loop invocation. This is because each line in the template gets transformed into a string that is included in the final output. While the over network size of this is fairly small at 18kb unzipping and parsing MBs of extra JS is going to have a significant performance penalty. I then used the CLI version of lighthouse to collect two stats: FCP => First Contentful Paint
The Results::
The template only component is so slow because it's necessary to use a I tested the Given the difficulty in maintaining a build time transform and the additional complexity this will add as ember moves towards embroiderer for a build. Plus the fact that minor versions of ember may included breaking changes to the glimmer API I'm tending towards closing this PR and instead adding a regular Am I reading these numbers correctly? Are there performance or DX issues I'm not thinking of? |
|
An option that brings the needed SVG glyphs into the page itself and references them from there, rather than externally, will work with IE11. I know this because we had to do this with the SVG icon symbol library (sprite) for our app. |
This adds a template placeholder
<FaStaticSprite>which will be transformed at build time into a plain DOM element with a link to the sprite for the icon.Eg:
becomes
Sprite files for included icon sets are added to
assets/fa-spritesautomatically.This realizes many of the benefits of the
enableExperimentalBuildTimeTransformwithout some of the drawbacks indicated in #117.I did some limited performance tests, rendering 3K coffee icons on the screen using this transform takes about 600ms whereas using the component it's around 3 seconds. So this is significantly faster at very large scale.