Issue #391: Add self-hosted font-awesome directly to Libki #393
Conversation
It appears that Font Awesome has implemented limits on the one free "kit" you can create. When the limit is hit for the month, the Font Awesome js file ( https://kit.fontawesome.com/61c1297cec.js ) returns a 403 error. This affects everyone running Libki. We should add the Font Awesome files directly to Libki to avoid this issue. Test plan: 1. Check out this branch 2. Clear any browser caching related to Libki running locally 3. Start libki-server 4. Verify font awesome icons are still displayed
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| console.log(`Font Awesome Free 5.15.4 by @fontawesome - https://fontawesome.com | ||
| License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL 1.1, Code: MIT License) | ||
| `) No newline at end of file |
There was a problem hiding this comment.
Logging detailed information such as software versions and licensing details in the console can expose the application to security vulnerabilities. This is particularly concerning if the application is deployed in a production environment where attackers might use this information to identify specific vulnerabilities related to the software version.
Recommendation: Consider removing or obfuscating these logs in the production environment to enhance security.
| font-family: 'Font Awesome 5 Brands'; | ||
| font-style: normal; | ||
| font-weight: 400; | ||
| font-display: block; |
There was a problem hiding this comment.
The use of font-display: block; can lead to a Flash of Invisible Text (FOIT) as the browser will block rendering of the text until the font is downloaded. This can affect the perceived performance of the webpage.
Recommended Solution: Consider using font-display: swap; which will display the text immediately with a fallback font and swap to the custom font once it has loaded. This improves the user experience by eliminating the invisible text during font loading.
| src: url("../webfonts/fa-regular-400.eot"); | ||
| src: url("../webfonts/fa-regular-400.eot?#iefix") format("embedded-opentype"), url("../webfonts/fa-regular-400.woff2") format("woff2"), url("../webfonts/fa-regular-400.woff") format("woff"), url("../webfonts/fa-regular-400.ttf") format("truetype"), url("../webfonts/fa-regular-400.svg#fontawesome") format("svg"); } |
There was a problem hiding this comment.
The order of font formats in the src property could be optimized for performance. Modern browsers support WOFF2, which is more efficient than other formats like EOT. Placing url("../webfonts/fa-regular-400.woff2") format("woff2") before the EOT format can help in faster font loading and rendering.
Recommended Change:
src: url("../webfonts/fa-regular-400.woff2") format("woff2"), url("../webfonts/fa-regular-400.eot?#iefix") format("embedded-opentype"), url("../webfonts/fa-regular-400.woff") format("woff"), url("../webfonts/fa-regular-400.ttf") format("truetype"), url("../webfonts/fa-regular-400.svg#fontawesome") format("svg");| font-family: 'Font Awesome 5 Free'; | ||
| font-style: normal; | ||
| font-weight: 400; | ||
| font-display: block; |
There was a problem hiding this comment.
The font-display: block; property is used, which keeps text invisible until the font is fully loaded. This can delay text rendering on slow networks. Using font-display: swap; can improve the user experience by showing a fallback font until the custom font is loaded, thus reducing the perceived load time.
Recommended Change:
font-display: swap;| font-family: 'Font Awesome 5 Free'; | ||
| font-style: normal; | ||
| font-weight: 900; | ||
| font-display: block; |
There was a problem hiding this comment.
The font-display: block; property is used here to control how text is displayed while the font is loading. This setting can cause a flash of invisible text (FOIT), which might not be desirable as it can affect the user's experience by delaying text visibility until the font is fully loaded.
Recommendation: Consider using font-display: swap; to allow text to be displayed with a fallback font while the font is loading, thus reducing the impact on the Cumulative Layout Shift (CLS) and improving the perceived performance.
| .fa.fa-remove:before { | ||
| content: "\f00d"; } | ||
|
|
||
| .fa.fa-close:before { | ||
| content: "\f00d"; } |
There was a problem hiding this comment.
There is redundancy in the CSS rules for .fa.fa-remove:before and .fa.fa-close:before, both setting content: "\f00d";. These could be combined into a single rule to reduce redundancy and streamline the CSS file. For example:
.fa.fa-remove:before, .fa.fa-close:before {
content: "\f00d";
}This change would maintain the same functionality while reducing the number of lines and improving readability.
| try { | ||
| if (typeof window !== 'undefined') _WINDOW = window; | ||
| if (typeof document !== 'undefined') _DOCUMENT = document; | ||
| } catch (e) {} |
There was a problem hiding this comment.
Global Variable Initialization
The script initializes global variables _WINDOW and _DOCUMENT based on the existence of window and document. This approach can lead to issues in non-browser environments where these objects might not exist or behave differently.
Recommendation:
Consider checking the environment more robustly or providing a more comprehensive fallback mechanism. For instance, you could use a factory function to generate these objects based on the environment, ensuring compatibility across different platforms.
| function str2rstrUTF8(input) { | ||
| return unescape(encodeURIComponent(input)); | ||
| } |
There was a problem hiding this comment.
Use of Deprecated JavaScript Function
The function unescape used in str2rstrUTF8 is deprecated and should not be used in modern JavaScript code. It can lead to issues with handling modern character sets and potentially open up security risks.
Recommendation:
Replace unescape with a more modern and secure function. For UTF-8 encoding, you can directly use decodeURIComponent or other secure libraries that handle character encoding more reliably and securely.
| } catch (e) { | ||
| if (!PRODUCTION) { | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
The error handling in the bunker function suppresses exceptions in production environments, which could obscure important errors and hinder debugging efforts. It's generally advisable to log these errors to an error monitoring service, even in production, to ensure that issues can be tracked and addressed.
Recommended Solution:
Consider logging the error using a monitoring tool like Sentry or New Relic, instead of completely suppressing it. This approach would allow for proactive issue resolution while keeping the production environment clean from unhandled exceptions.
| } else { | ||
| var _namespace$shims; | ||
|
|
||
| (_namespace$shims = namespace.shims).push.apply(_namespace$shims, shims); |
There was a problem hiding this comment.
Using the spread operator to append elements from one array to another inside a push method call can lead to performance issues if the shims array is large. This is because the spread operator in this context has to iterate over all elements of the shims array, which can be inefficient.
Recommended Solution:
To improve performance, consider using Array.prototype.concat() or a loop to handle the merging of arrays, especially if the size of the arrays can be large. This method can be more efficient and performant for large datasets.
Description by Korbit AI
What change is being made?
Replace external Font Awesome assets with self-hosted assets and wire up a conflict-detection mechanism for Font Awesome usage.
Why are these changes being made?
To ensure Font Awesome is fully self-hosted within Libki (reducing external dependencies) and to detect potential conflicts between webfont assets and other Font Awesome inclusions for safer, deterministic rendering. If applicable, this also makes licensing/attribution self-contained when bundling the assets.