Skip to content

POC: KvIcon2 - Dynamically import and inline svgs instead of using an SVG sprite#1541

Closed
ryan-ludwig wants to merge 2 commits into
masterfrom
VUE-223_kv-icon
Closed

POC: KvIcon2 - Dynamically import and inline svgs instead of using an SVG sprite#1541
ryan-ludwig wants to merge 2 commits into
masterfrom
VUE-223_kv-icon

Conversation

@ryan-ludwig
Copy link
Copy Markdown
Contributor

This will allow us to remove the SVG sprite which currently adds over 450 DOM nodes to every page.
Instead each page will only load the SVGs (inlined) that are on that page (if any).

Keeping the KvIcon component API the same should make the transition fairly simple.

Wanted to run this past the front-enders to get your thoughts first.

To implement, my plan is to

  • go through each instance of <kv-icon>
  • move the .svg file used to the /assets/inline-svgs/icons folder
  • switch it to kv-icon2
  • verify it looks the same as before, tweak CSS if needed (will probably need to add .my-icon { fill: $my-color } to many icons. Some already do this.
  • When finished, rename kv-icon2 to kv-icon, then find and replace all instances of kv-icon2 to kv-icon.
  • Remove the code for creating the old icon sprite

},
computed: {
iconFile() {
return () => import(`@/assets/inline-svgs/icons/${this.name}.svg`);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the magic happens.

@@ -5,7 +5,7 @@
autocomplete="off"
@submit.prevent="onSubmit"
>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation example. Really the only change in most cases will be adding the fill in CSS.

@emuvente
Copy link
Copy Markdown
Collaborator

I don't think it makes sense to do this for all icons, because it results in the svg code for the icon being repeated everywhere its used and included in every page load. This article has a good breakdown of the drawbacks and advantages of various techniques of using svg sprites. https://css-tricks.com/svg-sprites-use-better-icon-fonts/

/**
* Use KvIcon to display an inlined-svg from /assets/inline-svgs/icons on the page.
* SVG files should only be one color so they can be styled using CSS fill property: `.my-icon { fill: pink; }`
* Icons will fill the width of it's container. You can set the width with CSS: `.my-icon { width: 5rem; fill: pink; }`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not all icons use fill. Some use stroke. Others have multiple colors embed...

@@ -0,0 +1,3 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 22 22">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these extra files required if we already have individual files in the assets icons folder?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The special inline-svg webpack loader only pulls from /assets/inline-svgs/**/*.
We could just dump everything from /assets/icons into /assets/inline-svgs/icons, but I'd rather just move what we're actually using by hand to remove cruft.

}

.search-icon {
fill: $kiva-text-dark;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move the base styles for icon sets (some stroke, some fills) out of KvIcon to a global inclusion so these individual tweaks wouldn't be required unless you want a special color. It's not very much css.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not necessary. The css will only be included once per page, as css is included per component definition, not per component instance. Also, as this is in src/components it's probably already being bundled into the global css.

@ryan-ludwig
Copy link
Copy Markdown
Contributor Author

I don't think it makes sense to do this for all icons, because it results in the svg code for the icon being repeated everywhere its used and included in every page load. This article has a good breakdown of the drawbacks and advantages of various techniques of using svg sprites. https://css-tricks.com/svg-sprites-use-better-icon-fonts/

The big performance improvement with this dynamic import method is that most of our svgs won't be included on every page load.

An example scenario:
Page1 uses icons x, y, z
Page2 uses icons a, b, b, b, b, b, c, d
Page3 uses no icons

In the sprite method, Page1, Page2, and Page3, all make a network request for the sprite and load a, b, c, d, x, y, z onto the DOM regardless of what they use.

In the dynamic import method there are no network requests, the server sends the SVG inlined with the HTML via SSR.
Page1 will show icons x, y, and won't load a, b, c, d onto the DOM.
Page2 will see icon b's SVG code repeated in HTML 5 times. In this scenario a sprite could arguably be better. But it's still adding icons x, y, and z to the dom even though they aren't used. So it's kindof a wash. The math would depend on how many svgs you have in the sprite and how many times the same icon is repeated on a page. Some more info here.
Page3 will have no requests and no icon DOM nodes.

I've used sprites in the past, while they're really good when you repeat an icon lots times on one page, they're usually just loading a lot of stuff that isn't relevant for that page/component, and tend to bloat over time.

Many component libraries in React, Vue, etc. have moved to importing just what they need either as vue/react components or as svgs:
Material UI
https://material-ui.com/components/icons/#material-icons

IBM's carbon design system
https://github.com/carbon-design-system/carbon/tree/master/packages/icons-vue

Attlasian:
https://atlaskit.atlassian.com/packages/core/icon

React icons:
https://github.com/react-icons/react-icons

There are some libraries like Vuetify that are still using sprites or even icon fonts 🤷‍♂️, but trend is moving away from that. Components are great at encapsulating what's needed and avoiding global stuff like sprites.

@emuvente
Copy link
Copy Markdown
Collaborator

@ryan-ludwig While that's a good example, we don't actually have any pages that don't load icons (because of the header). The triangle and chevron icons are used many times in every page (especially mobile/small screens). The star icon is almost always used multiple times in a page when it's used (either as the favorite button on every loan, or for showing the risk rating). Sprite sheets take advantage of browser caching, so there's only one request ever needed for an entire browsing session. We definitely shouldn't be putting everything in a sprite sheet, but we also definitely shouldn't be inlining everything. We could rename the KvIcon component to something like KvSprite and be more stringent about what is inlined and what is added to the sprite sheet.

@ryan-ludwig ryan-ludwig requested a review from mcstover March 13, 2020 15:39
@ryan-ludwig
Copy link
Copy Markdown
Contributor Author

@emuvente That's a good point about the chevrons and triangles, I think making a very slim KvSprite is a good plan :) If we can ultimately reduce the number DOM nodes on the page site-wide, that'd be a good win. We get dinged on lighthouse for the excessive DOM, the current homepage has 832 nodes, of those 465 are from the sprite. I'll try making a slim sprite today and see if we can get the sprite under 50 nodes.

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.

3 participants