10up / Engineering-Best-Practices

10up Engineering Best Practices
https://10up.github.io/Engineering-Best-Practices/
MIT License
757 stars 205 forks source link

Images - SVGs #102

Open zrothauser opened 9 years ago

zrothauser commented 9 years ago

Splitting off the discussion from #101 into separate discussions:

Our current statement on SVGs is "When using SVG you should always provide a fallback such as a PNG image for browsers that do not support vector graphics."

@daveross and @jonathantneal both questioned the need for PNG fallbacks, and @jonathantneal provided a possible update to the SVG wording:

Use SVG images whenever possible. They are resolution independent, easily styleable with CSS, and often smaller and clearer than other image formats.

When using SVGs as an icon system, combine them into a single sprite sheet that allows individual images be displayed with an ID.

When using SVGs and supporting Internet Explorer 8, provide a fallback, such as a PNG image.

morganestes commented 9 years ago

When using SVGs as an icon system, combine them into a single sprite sheet that allows individual images be displayed with an ID.

I vote for replacing "an ID" with "a class" or "an attribute" (like data-icon or something of the sort) so it's clear that you can use it multiple places on a page.

jonathantneal commented 9 years ago

My wording could be improved, because I think you’ve confused IDs in the SVG with IDs on the page.

When using SVGs as an icon system, combine them into a single sprite sheet that allows individual images be displayed with an fragment identifier (an ID).

The document:

<p>
  <svg><use xlink:href="sprites.svg#thing"/></svg>
</p>

The "sprite.svg" file:

<svg>
  <symbol id="thing"/>
</svg>

See https://css-tricks.com/svg-symbol-good-choice-icons/

morganestes commented 9 years ago

Derp! Yep, I got confrused there, and I'm guessing others who aren't intimately familiar with SVG syntax will, also. Honestly, I think I've only used inline SVG one time years ago, and I sometimes forget that the <svg> element/object exists to be used this way.

tddewey commented 9 years ago

Thar be dragons with svg :)

Still, I think we need to be pushing to use it for our sites as the advantages are real.

I'd recommend we write something into our best practices that is pretty strongly worded in favor of utilizing svg if IE8 is not a concern (based on real browser usage stats).

Even with modern browsers though, there are still a ton of pitfalls (just try to use the "use" tag without a polyfill!). So ultimately, I think there needs to be room to fallback to less modern development methods if the budget or timeline necessitate it.

With any discussion of SVGs should be mention of commonly used/approved polyfills. (e.g. @jonathantneal's SVG for everybody) and/or various grunt fallback workflows. That way everyone is working with SVGs in a similar way.

tlovett1 commented 9 years ago

@tddewey totally agree. Excited to see a PR for this one.

saltcod commented 8 years ago

Hey all,

I'd like to put together a PR for this over the next week or so.

While I do that, I thought I'd outline a few of the bigger questions I have:

And that mostly covers it for the moment. Thanks all — love to get any feedback if you have it.

nprasath002 commented 6 years ago

Looks like SVG is supported in all browsers now https://caniuse.com/#feat=svg

dana-ross commented 6 years ago

Indeed. SVG support is really good in 2018.

We did have a discussion come up in a project recently about SVG security, since SVG does allow for embedding JavaScript. Browsers will ignore code in an SVG if it's included in an <img> tag. SVGs included directly in the page markup (say, to make rendering icons faster) still get code executed. We should note that in any SVG guidance we include.

If someone does want to include SVG images inline, they need to be checked for a payload before including them in the theme.

timwright12 commented 5 years ago

@saltcod did we ever get a PR for this? If not I think we can get something moving forward

saltcod commented 5 years ago

I'd like to put together a PR for this over the next week or so.

@timwright12 That's from 2016! Making this the longest week in history!

I should be able to put together a PR this week. Given that this issue is 3 years old, I'll poll the room and see what direction we should move in in 2018.

jonathantneal commented 5 years ago

Oh, I’ve narrowed my position since last week, Terry. 😄

  1. I don’t think we should mention PNG fallbacks at all. I think they are irrelevant in an IE9+ world.
  2. I think source SVGs should be maintained as separate files, compiled into a map of <symbol id> sprites, or compiled inline with HTML using the project’s HTML/JSX compiler.

My position hasn’t changed here, though — whatever works best for your team is probably best!

saltcod commented 5 years ago

haha sweet! Thanks for stopping in! Agreed on png fallbacks — I haven't thought about those in a long time.

I think most of the FEE team here still uses the (PHP) function that I'm pretty sure you wrote.

The one that lets you do something like svg_icon( 'logo', array ( ...classes, attributes, etc.. ) )

Probably a question for @timwright12: Should we include this actual function in the documentation or simply talk about how we would use this workflow in more general terms?