lookbook-hq / lookbook

A UI development environment for Ruby on Rails apps ✨
https://lookbook.build
MIT License
889 stars 91 forks source link

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

Closed camertron closed 4 months ago

camertron commented 4 months ago

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 😄

netlify[bot] commented 4 months ago

Deploy Preview for lookbook-docs canceled.

Name Link
Latest commit 8b4c3757d041825fbbd5ed4df1bd052540c09a78
Latest deploy log https://app.netlify.com/sites/lookbook-docs/deploys/664c27394d44110008feaec2
allmarkedup commented 4 months ago

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 ahuge 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 commented 4 months ago

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 :)

camertron commented 4 months ago

Awesome, thank you!