search

vhx / quartz-react

Quartz components using React
2 stars 1 forks source link

Add SVG Icons #51

Open sebastiansandqvist opened 7 years ago

sebastiansandqvist commented 7 years ago

Overview

This PR adds the SVG files for the icons used throughout Quartz. It does not yet implement anything that allows the icons to be used. See options below...

This PR can be used for discussion. I outline 3 approaches I think would be appropriate, but these need consideration before any one of them is implemented.

Current approach

All of the icons get run through a process that inlines them (in base64) into a css file. They also get manually assigned a color, so .icon-product--teal is almost a complete duplicate of .icon-product--gray, with a slightly different base64 representation to change the fill property.

Proposed approaches

  1. Use <img src='/icons/product.svg' className='icon icon--teal'/> in jsx. This is great for caching and requires almost no engineering effort. It's supported back to IE9. Instead of creating duplicate icons, we can one-time define icon--teal and similar classnames to set the appropriate svg fill color.
  2. Use .icon--product { background: url('/icons/product.svg') } in css. Same benefits as option 1. Also supported all the way back to IE9.
  3. Use .icon--product { background: url('data:image/svg+xml; ...') } in css. Using base64 to inline the icons in css is worse for caching but better in that it saves an http request to fetch the svg. We can still use the class names that set the fill so that we don't have to duplicate each icon's path data for every color. Aside from the cache/request tradeoff, the only downside is that this will require a build step to handle converting to base64.

Options 1 and 2 can optionally reference the svgs from a cdn rather than paths on our server.

Other approaches not considered

davegonzalez commented 7 years ago

I'd rather not use option 3. If we can get away with one less build step let's do it. Also, Base64'ing seems like it may be slightly finnicky: https://css-tricks.com/probably-dont-base64-svg/

I like option 2 the most in a general use case (outside of these components) but I can see how option 1 would be best for components. Since we're working towards using react for everything, maybe it's best to just go with #1? @erikmoldovan or @ninjaofawesome ?

sebastiansandqvist commented 7 years ago

This update could actually be done independent of any major CSS changes. Only the <Icon /> component would need to be rewritten in order to handle whatever the new functionality happens to be.

My vote is for option 1 above. If there is an [icon-name].svg file for every icon listed in this icon-list file the <Icon> could be a very minimal component that only adds classes and provides <img src={${iconName}.svg} alt={iconName} />. The tricky thing then would be that the location of the icon ought to be flexible. If that is not practical to ensure, then option 2 might be better. I agree base64 is not my favorite of the three ideas. There also may be another possibility.