perf: use svg sprite for icons #16135
Conversation
|
@G3root is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
bot
added
the
performance
Graphite Automations"Add community label" took an action on this PR • (08/08/24)1 label was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (08/08/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (08/08/24)1 reviewer was added to this PR based on Keith Williams's automation. |
| import glob from "fast-glob"; | ||
| import fs from "fs-extra"; | ||
| import path from "node:path"; | ||
|
|
There was a problem hiding this comment.
script to import svg from lucide icons
| // icon name from lucide-react | ||
| // this list is extracted based on what icons are currently used | ||
| export const lucideIconList = new Set([ | ||
| "x", |
There was a problem hiding this comment.
list of icon names used throughout the app
There was a problem hiding this comment.
if we want to add a new icon, we have to add it here?
| @@ -0,0 +1,641 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
sprite sheet generated from build-icons command
packages/ui/package.json
Outdated
| @@ -20,7 +20,8 @@ | |||
| "type-check": "tsc --pretty --noEmit", | |||
| "type-check:ci": "tsc-absolute --pretty --noEmit", | |||
| "lint:fix": "eslint . --fix", | |||
| "lint:report": "eslint . --format json --output-file ../../lint-results/ui.json" | |||
| "lint:report": "eslint . --format json --output-file ../../lint-results/ui.json", | |||
| "build-icons": "node ./scripts/build-icons.mjs" | |||
There was a problem hiding this comment.
command to generate the sprite sheet when new icons are added to the list
|
requesting @zomars review here as he worked on the Icon component the most |
|
Got some missing icons |
packages/ui/package.json
Outdated
| @@ -20,7 +20,8 @@ | |||
| "type-check": "tsc --pretty --noEmit", | |||
| "type-check:ci": "tsc-absolute --pretty --noEmit", | |||
| "lint:fix": "eslint . --fix", | |||
| "lint:report": "eslint . --format json --output-file ../../lint-results/ui.json" | |||
| "lint:report": "eslint . --format json --output-file ../../lint-results/ui.json", | |||
| "build-icons": "node ./scripts/build-icons.mjs" | |||
There was a problem hiding this comment.
| "build-icons": "node ./scripts/build-icons.mjs" | |
| "build:icons": "node ./scripts/build-icons.mjs" |
To match the READMEs and comments
|
|
do we know the performance difference @G3root ?? |
|
@PeerRich here's the breakdown
before
after
|
|
Here’s an example of before and after. Before, when refreshing the page, the icons would flicker and show a skeleton. After, the icons no longer flash during refresh. there are some frame drops in the video due to my screen recorder beforeScreencast.2024-08-09.20.24.56.webmafterScreencast.2024-08-09.20.41.25.webm |
|
@G3root what if we want to use the |
|
Just a heads up there's a fix for SVG and CORS in here #16244 |
What does this PR do?
Rendering icons as JSX components is expensive and significantly increases the size of your bundle. This PR aims to address that by replacing them with SVG sprite icons, which are cacheable and faster than the JS based solution.
here's a explanation why we shouldn't import SVG icons as JSX https://twitter.com/_developit/status/1382838799420514317?lang=en
Note
Since this change is one-to-one with the previous implementation, it might still introduce some regressions, so it needs to be tested thoroughly
here's how it's looks like after the changes
Screencast.2024-08-08.18.45.30.webm
Mandatory Tasks (DO NOT REMOVE)