Switch from fontawesome webfonts to SVG + JS #1619
Conversation
https://discourse.pi-hole.net/t/use-svg-for-icons-instead-of-font/40166 Signed-off-by: Michael Howell <[email protected]>
|
Thank you for the submission and the detailed explanation why it's needed. I'm not much of a JS guy so I can't attest to the code itself, but I like the concept. |
|
@XhmikosR Can you take a look over this, please? This seems in your wheelhouse. |
|
I'm not sold on the benefits of this, personally. I still use fonts and I have no issue with this approach. That being said, this change should be made upstream too so that it reaches a bigger audience and more people benefit. Last time I checked, AdminLTE 3.x used the CSS. But if this is accepted upstream, one more reason to go with it here too. Assuming all instances are changed, and someone tests this, it's your call, at the end. I don't have a lot of time to test this myself. |
|
Also, the less JS, the better. I haven't measured this at all nor how much it affects the page size, but JS needs to be parsed and executed. Just my 2 cents :) |
|
I agree with XhmikosR that this should be a change at the upstream level. But in this case, we are the upstream since we've forked. The vue node package is moving to SVG+JS and has noted no bad effects. https://www.npmjs.com/package/@fortawesome/vue-fontawesome#learn-about-our-new-svg-implementation |
They added support for it already. |
|
This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there: https://discourse.pi-hole.net/t/use-svg-for-icons-instead-of-font/40166/7 |
|
This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there: https://discourse.pi-hole.net/t/pi-hole-core-web-v5-2-2-and-ftl-v5-3-3-released/41998/1 |
By submitting this pull request, I confirm the following:
git rebase)git commit --signoff)What does this PR aim to accomplish?:
There are several problems with icon fonts. My biggest problem is that I see a box with
F104in it where there ought to be a picture, because I have web fonts disabled (for the same reason I use Pi-hole; most of the time, web fonts are a waste of time and they're in the critical path for rendering the page). Other people disable web fonts because they're dyslexic and want to have custom fonts used all the time, which is a far more important reason to make sure your site is usable without web fonts.Web fonts are also anti-aliased as fonts, which can look ugly. Basically, using fonts for icons results in a bunch of stuff not really treating them correctly.
How does this PR accomplish the above?:
It uses Font Awesome's SVG + JS configuration. Since the Pi-hole dashboard already requires JavaScript to work at all, and it's very close to being a total drop-in replacement for the older CSS + Web Font setup, it seemed like the best choice with the least impact on the rest of the code. It's also supported by FontAwesome, and they have other nice features built into it.
The biggest downside is that it's a non-async JavaScript. If I had made it async, or loaded the script at the bottom of the page instead of the top, you'd have gotten a bit of jank when the script finished loading. The way to avoid both jank and avoid forcing the icons to load before the page becomes visible by inlining the icons or by using the plain SVG sprite, but that would have been a more invasive change to the AdminLTE code. At least this way, it only loads one file (a giant JavaScript file) instead of two (the CSS and the Web Font).
Before:
After:
What documentation changes (if any) are needed to support this PR?:
This should have no user-visible changes, except making things work in configurations where they used to be broken.