Skip to content
This repository has been archived by the owner on Jan 10, 2018. It is now read-only.

SVG in place if icon font #60

Closed
mor10 opened this issue Sep 11, 2016 · 19 comments
Closed

SVG in place if icon font #60

mor10 opened this issue Sep 11, 2016 · 19 comments

Comments

@mor10
Copy link
Contributor

mor10 commented Sep 11, 2016

SVG is the emerging standard for displaying scalable icons, taking over for icon fonts. They are true icons, more acceasible, and allow for more flexibility in terms if style and layout.

This would be a good place to introduce a best-practice example of SVGs in a theme.

The only real argument against SVG at this point is backwards compatibility. This can be solved with Grunticon and other tools.

@melchoyce
Copy link
Contributor

+1. @davidakennedy and I have been chatting about this as well and we think it's the right theme to ditch icon fonts. :)

@samikeijonen
Copy link
Contributor

I have been using SVG icons in my latest public themes. I'd love to contribute on this one if you want. My starting point have been how wd_s theme uses SVGs. And here is SVG related functions that I have used in public themes.

It covers at least these areas:

  1. How to load SVG sprite in the footer.
  2. Helper function for SVG markup which can be use anywhere we have control of the markup.
  3. Using SVG icons in Social menu.
  4. Adding arrow icon as a dropdown indicator in menus.

Just let me know if there is something where we can start.

@mor10
Copy link
Contributor Author

mor10 commented Sep 11, 2016

Very nice @samikeijonen! I have not tested this, so I'll just ask the question: Does this inject the SVG as an inline element, or reference it?

@samikeijonen
Copy link
Contributor

samikeijonen commented Sep 11, 2016

I'm not sure about correct English terms but there are two ways I have injected SVGs.

  1. Inline from SVG sprite. prefix_get_svg function outputs SVG like this:
<svg class="icon icon-github" aria-hidden="true" role="img">
   <use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#icon-github"></use
</svg>

In this method we include SVG sprite in the wp_footer. This is the method I'm using in Checathlon theme.

Note that in the Customizer I use external reference, otherwise SVG icons won't show up in the preview. There is a core ticket for this.

  1. Use external reference from sprite everywhere. In this method we need to use svguse.js for IE browsers. Also in this method we don't need to include SVG sprite before </body> element. Output would be like this:
<svg class="icon icon-twitter">
  <use xlink:href="path/to/icons.svg#twitter-icon"></use>
<svg>

This is the method I use in Some theme at the moment.

@mor10
Copy link
Contributor Author

mor10 commented Sep 12, 2016

Either one of these approaches will work, provided there are proper fallbacks through some form of JS inclusion. The main point is to ensure the SVGs are true elements in the markup, not merely background images. There are many paths that lead to that result. Yours looks like a pretty solid start.

@samikeijonen
Copy link
Contributor

Do you want me to create PR for Social menu using SVGs so we could start somewhere. This is also related to #65.

@mor10
Copy link
Contributor Author

mor10 commented Sep 16, 2016

I'm inclined to say yes, but I think we should hold off until Mel gives the all-clear.

@melchoyce
Copy link
Contributor

Let's hold off on this pending #65, since I'm not sure yet which icons we're going to be using.

@samikeijonen
Copy link
Contributor

Hi @melchoyce. Just to make sure, I have couple of questions:

  1. We go with Font Awesome SVG icons?
  2. Is the list of needed icons still this?
  3. Should I create PR only for SVG related functions, or also CSS styles related to SVG icons?

@samikeijonen
Copy link
Contributor

I created PR #206 where I only touched social links menu. Let me know where you want to continue discussion.

Fallback for IE8 I was thinking using feature.js or similar lightweight library. We could detect if browser don't support SVGs and do couple of things:

  1. Hide the icon using display: none.
  2. Show the text inside screen-reader-text class.

That would mean text fallback for IE8.

@melchoyce
Copy link
Contributor

melchoyce commented Oct 8, 2016

We go with Font Awesome SVG icons?

Correct

Is the list of needed icons still this?

Yes

We'll also want to add social icons: http://fontawesome.io/icons/#brand There's a lot of social icons there and I don't think we need all of them. I'll put together a list of the ones I think are most relevant to the theme in another ticket. Keep an eye out for that ticket and ignore social for now.

Should I create PR only for SVG related functions, or also CSS styles related to SVG icons?

I think the PR should include everything the SVGs need to display properly, so CSS is probably appropriate.

@melchoyce
Copy link
Contributor

melchoyce commented Oct 8, 2016

Sorry, GitHub didn't load your latest comment so I didn't see you started with Social... I guess that's okay, we'll just want to revisit the list because Font Awesome has much better social support than Genericons, which that list is based off of. Can we focus on getting the theme icons in asap?

@samikeijonen
Copy link
Contributor

Can we focus on getting the theme icons in asap?

Do you mean removing all Genericons and replacing them using SVG icons?

In #206 I'm already using Font Awesome SVG icons.

Also, how much time do we have before this should be done?

@melchoyce
Copy link
Contributor

Do you mean removing all Genericons and replacing them using SVG icons?
In #206 I'm already using Font Awesome SVG icons.

Ah — Confused because it the PR title just say "Social Menus." Can you update the title to reflect that it's all of the SVGs, just so that's clear?

Also, how much time do we have before this should be done?

@davidakennedy what are your thoughts on timeline?

@samikeijonen
Copy link
Contributor

samikeijonen commented Oct 8, 2016

Sorry, I was still unclear. PR #206 is for using SVG icons just in social links menu at the moment. It doesn't touch other icons at all.

If you want I can try to replace all Genericons and use SVGs instead in the same PR.

I know it takes time, that's why asked about the timeline.

@grappler
Copy link
Member

grappler commented Oct 9, 2016

plan is theme merging into core by October 19th with enhancements needing to complete by October 26th

Quoated from https://make.wordpress.org/core/2016/10/07/dev-chat-summary-october-5-4-7-week-7/

@samikeijonen
Copy link
Contributor

Thanks Ulrich.

I updated PR #206 replacing all Genericons with SVG icons. Check the comments in there.

@melchoyce
Copy link
Contributor

Thanks @samikeijonen!

I'll make a new issue to discuss social icon support.

@davidakennedy
Copy link
Collaborator

Thanks to all who have worked on this issue and pushed it forward! Thanks to @samikeijonen for leading the way with the code. In short, it's going in, and I'll work on it today.

A fallback method will be needed and after reading through this post on fallbacks, it seems like the one with the widest reach will be an image fallback inside the inline SVGs. That's also nice because it does not require extra JavaScript. So that's the direction I'm leaning along with @laurelfulford, but we're happy to hear any benefits and drawbacks to this and other methods.

This will be the first default theme that uses SVGs in a big way, so that's cool! ❤️

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

No branches or pull requests

5 participants