Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the Lucide sprite instead of injecting individual SVGs onto the page #616

Merged
merged 2 commits into from
May 21, 2024

Conversation

camertron
Copy link
Contributor

@camertron camertron commented May 21, 2024

Hey there @allmarkedup 👋 it's me again 😄

A co-worker noticed the Primer Lookbook instance loading quite slowly and discovered there are ~600 SVGs on many of our pages. It looks like Lookbook copies the SVG source onto the page for every icon, which the co-worker surmised is leading to long render times on the server. I noticed Lookbook comes with a sprite that includes all the Lucide icons in the img/ directory, so I thought maybe we could just use that instead?

I tried replacing the SVG source with a <use> element, and it seems to work nicely. Page render times seem to be a little speedier too, although I won't know how much impact it has on Primer's Lookbook instance until we deploy to prod.

Let me know what you think 😄

Copy link

netlify bot commented May 21, 2024

Deploy Preview for lookbook-docs canceled.

Name Link
🔨 Latest commit 8b4c375
🔍 Latest deploy log https://app.netlify.com/sites/lookbook-docs/deploys/664c27394d44110008feaec2

@camertron camertron changed the title Use the Lucide sprite instead of injecting separate SVGs onto the page Use the Lucide sprite instead of injecting individual SVGs onto the page May 21, 2024
@allmarkedup
Copy link
Collaborator

Hey @camertron, thanks for this!

So funnily enough, this is exactly how Lookbook used to serve icons - that is why that sprite is there :) But I wasn't too happy about sending a 300+kb sprite over the wire when the UI only used a handful of different icons, so I switched to inlining the SVGs instead.

However... I suspect what I hadn't accounted for is that for apps with large amounts of previews will have a lot of nav items, each of which contains a few icons which if they are all inlined could definitely start making the size of the DOM quite chunky. I've just 'viewed-source' on the Primer Lookbook UI and it's definitely pretty huge which will for sure affect performance of page updates.

As the sprite will get cached but the inlined SVGs won't I guess it does make sense to go back to this way of doing things, especially to benefit the larger Lookbook installs. And to be honest at some point in the future I'm sure it wouldn't be too difficult to generate a custom sprite with just the subset of icons that Lookbook actually uses.

I'm not sure it will make a_huge_ difference to performance but every little helps. I'll also try to have a think about other ways that performance could be improved for larger installs too, it's not something I've put a lot of time into up until now if I'm honest.

I'll give this a test and unless I run into any issues I'll merge it in and get it out in the next release. Thank you!

@allmarkedup
Copy link
Collaborator

All looking good @camertron 👍

I'm going to merge this in now and when I have a chance I'll try to have a play with putting together a build task for a custom sprite with just the icons that are actually used in the UI.

Thanks for the PR and let me know if you think it makes any difference to the UI sluggishness. I've got a few other ideas for improving performance that I'm going to have a think about too, if I get anything concrete together I'll let you know :)

@allmarkedup allmarkedup merged commit ad1ba3b into main May 21, 2024
16 checks passed
@allmarkedup allmarkedup deleted the lucide_sprite branch May 21, 2024 20:03
@camertron
Copy link
Contributor Author

Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants