Use embedded Font Awesome SVG icons #2484
Conversation
Co-authored-by: Michael Howell <[email protected]>
Co-authored-by: Michael Howell <[email protected]>
|
Would it be possible to get a review on this PR? I don't want it to lose momentum and have to deal with conflicts again down the road. |
| fa::svg(type_, &icon).or_else(|_| fa::svg(fa::Type::Brands, &icon)) | ||
| { | ||
| format!( | ||
| r#"<span{before}class="fa-svg{other_classes}"{after}>{svg}</span>"#, |
There was a problem hiding this comment.
Wouldn't it be better instead of generating the SVG in the DOM directly to rely on CSS instead? It brings multiple advantages:
- Smaller DOM
- If the same SVG is used more than once, it's not duplicated
There was a problem hiding this comment.
I don't want to build a giant stylesheet with every icon in it unless we have to. That file would be immense, and most of it wouldn't get used. But mdBook exposes an API that lets the user include any icon they want, so we'd have to?
Besides, fontawesome SVGs almost all have a single <path> in them. The DOM is tiny.
There was a problem hiding this comment.
FontAwesome is a font. Meaning that as long as we have the font, we have easily use their icons without needing to declare them ourselves.
There was a problem hiding this comment.
FontAwesome is a font. Meaning that as long as we have the font, we have easily use their icons without needing to declare them ourselves.
But then you must include the entirety of the font (which is quite large iirc), unless you want to deal with subsetting? And then that is a whole other ordeal.
|
Overall I think it's a very good idea, but unless I'm wrong, you're inlining the SVGs directly into the HTML, and I don't think this is the right approach. If you use the CSS and font provided by font awesome, then no need for this. |
CSS is a valid way to go about the inclusion of icons, and could be done in the future, but it should not be a blocker for this pull request to land. Much of the point of this pull request is to avoid the 77kb of fonts that must be loaded from FontAwesome, and to avoid the other downsides as mentioned in #1330 (comment). |
|
Then please inline the SVG into the CSS to prevent duplication in HTML. |
Incorporates the changes from #1330, brings up to date with repository and adds various fixes & improvements.