Skip to content
This repository has been archived by the owner on Feb 28, 2022. It is now read-only.

use <svg> instead of <img> for icons #509

Closed
rofe opened this issue Oct 24, 2019 · 18 comments · Fixed by #521
Closed

use <svg> instead of <img> for icons #509

rofe opened this issue Oct 24, 2019 · 18 comments · Fixed by #521
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers released

Comments

@rofe
Copy link
Contributor

rofe commented Oct 24, 2019

We are currently using <img> tags to display SVG icons. This works fine, it just doesn't allow styling of the contents (e.g. changing the color of logo). We should either switch to <svg> (breaking change) or make it configurable and keep <img> as default.

@rofe rofe added enhancement New feature or request good first issue Good for newcomers labels Oct 24, 2019
@rofe
Copy link
Contributor Author

rofe commented Oct 24, 2019

@anfibiacreativa

@davidnuescheler
Copy link
Contributor

davidnuescheler commented Oct 29, 2019

i definitely agree and would like to refer to the initial #500 issue. beyond symbol and use i would still advocate for a single icons.svg sheet. were there any objections to the original markup as proposed ... eg:

<svg class="icon twitter"><use xlink:href="/icons.svg#twitter"></use></svg>

... or is that just a function of a library that we ended up using?

@rofe
Copy link
Contributor Author

rofe commented Oct 29, 2019

... or is that just a function of a library that we ended up using?

We ended up implementing it without any libraries, and besides the fact that it was quicker to create single SVGs for the demo ;) @tripodsan and @trieloff actually both found having a folder with individual icons more sustainable/better manageable. As I suggested in #500 we could support both (auto-detect or make configurable), but I also think that we should have a discussion about pros and cons of each approach...

@trieloff
Copy link
Contributor

trieloff commented Oct 30, 2019

Single SVG file:

  • hard to manage, as every update changes the entire file
  • moderately easy to deliver (pro: single request, con: lots of dead weight for large icon libraries)

Multiple SVG files:

  • easy to manage (I recall a discussion with @davidnuescheler about the intuitiveness of may small files vs. one big file with the same contents)
  • easy to delivery (no negative performance impact with HTTP/2)

Last week I thought that we might compile the entire icons folder into a single icons.svg file for delivery, but this does not seem like a good idea anymore. I think the advantages of the multi-file approach are very stark.

@davidnuescheler
Copy link
Contributor

well, let me just state that from my manual experience working with icons as symbols, i have always gravitated to one file for manual management reasons.
to me it almost feels like a css file at that point and it seems that a large number of super small files (effectively 1-3 lines) is not really that helpful in css either.

i almost agree on the http/2 comment, except that the overhead even within the svg file considering the usual svg export clutter that's actually only mildly helpful for most icons is considerable. compare for example:

<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 23.1.1, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
	 viewBox="0 0 22.2 22.2" style="enable-background:new 0 0 22.2 22.2;" xml:space="preserve">
<style type="text/css">
	.st0{fill:#FF0000;}
</style>
<g>
	<polygon class="st0" points="13.8,1.2 22.1,20.7 22.1,1.2 	"/>
	<polygon class="st0" points="0,1.2 0,20.7 8.1,1.2 	"/>
	<polygon class="st0" points="7.4,16.7 11.3,16.7 12.8,20.7 16.2,20.7 11,8.3 	"/>
</g>
</svg>

... to ...

<symbol id="adobe" viewBox="0 0 22.2 22.2">
	<polygon class="st0" points="13.8,1.2 22.1,20.7 22.1,1.2"/>
	<polygon class="st0" points="0,1.2 0,20.7 8.1,1.2"/>
	<polygon class="st0" points="7.4,16.7 11.3,16.7 12.8,20.7 16.2,20.7 11,8.3"/>
</symbol>

also, as most generated svgs don't have accessibility built in, i think taking an actual look at the svg code is very desirable in most cases and add for example title and description... so manually adding an icon is suggesting some discipline, and possibly even reconsider the viewBox.

<symbol id="adobe" viewBox="0 0 22.2 22.2">
        <title>Adobe</title> 
        <desc>Adobe Logo</desc>
	<polygon class="st0" points="13.8,1.2 22.1,20.7 22.1,1.2"/>
	<polygon class="st0" points="0,1.2 0,20.7 8.1,1.2"/>
	<polygon class="st0" points="7.4,16.7 11.3,16.7 12.8,20.7 16.2,20.7 11,8.3"/>
</symbol>

i think bottom-line is, that i think that svg icons are used so frequently that they deserve a little bit of care and having a human developer look at the code at least once is probably a good idea.

@trieloff
Copy link
Contributor

Hm. I've never seen anyone touch an SVG file directly and would assume that most SVGs we deal with have been created by Illustrator. I might be pessimistic, but it feels like we are betting against human laziness here.

@davidnuescheler
Copy link
Contributor

...considering that the source of this conversation started with https://theblog.adobe.com/ feel free to look at html source for the icons and you will see that they are even inlined in the html.
i think svg really has become a more vital part of front end development, much more than an image format.

@anfibiacreativa
Copy link
Member

Hi David. I definitely agree with that last statement. Not so sure about using svg attributes for accessibility. In general, stripping svgs from them before generating a sprite, tends to be a convention. (Usually with https://github.com/svg/svgo or similar) If the icons are decorative, titles and other information are not required for compliance with WCAG. When I think about it, from an operative perspective, it would be ideal for the authors to be able to maintain icons in a folder, as Lars suggested, but there should be an automated task to generate a new sprite when the contents of that folder change.

@davidnuescheler
Copy link
Contributor

i definitely understand the pre http/2 sprite generation for delivery performance, but i am not sure that this applies in an http/2 age anymore, i think that more general development paradigm issue i have with pre-processors, minification etc. and frankly all deployment tasks.

i am not entirely sure how to read you accessibility comment. are you saying that having title and description in the svg symbol is the wrong place, or there shouldn't be a title or description? i think in the usecase we have at hand we have symbols for social, logos, products etc. (eg. https://helix-norddal-anfibiacreativa.hlx.page/ at the bottom) titles and descriptions would be super helpful.

@anfibiacreativa
Copy link
Member

I have to agree with you that, in a system that guarantees shipping over http/2, there is no need for sprite generation. So, it's true that svgs can be requested on an individual basis, and that should not be an issue.
When it comes to accessibility, I was not sure if assistive technology could read that title and description from the svg tag itself. According to this page https://a11y-101.com/development/svg it's possible, as long as the role="img" is present in the tag. With that, it definitely makes sense and are indeed helpful.

@rofe
Copy link
Contributor Author

rofe commented Nov 8, 2019

In an effort to refocus this issue on the original question which tag to use for icons in the output, I created follow up issues based on the discussion above:

@rofe rofe closed this as completed in #521 Nov 9, 2019
rofe added a commit that referenced this issue Nov 9, 2019
BREAKING CHANGE: icons are now rendered as <svg> instead of <img> tags.
If icons have been used and styled before, CSS changes are required.
adobe-bot pushed a commit that referenced this issue Nov 9, 2019
# [6.0.0](v5.6.4...v6.0.0) (2019-11-09)

### Features

* **icons:** use <svg> instead of <img> tag ([#509](#509)) ([5e43175](5e43175))

### BREAKING CHANGES

* **icons:** icons are now rendered as <svg> instead of <img> tags.
If icons have been used and styled before, CSS changes are required.
@adobe-bot
Copy link

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@trieloff
Copy link
Contributor

@rofe do we really consider this a breaking change? Is any HTML change a breaking change? If yes, we should probably document this in CONTRIBUTING.md

@rofe
Copy link
Contributor Author

rofe commented Nov 12, 2019

@trieloff I personally think yes, because it will affect customer CSS. But I'm open to being convinced otherwise :)

@trieloff
Copy link
Contributor

I think it makes sense.

@davidnuescheler
Copy link
Contributor

currently we seem to render something like:

<svg xmlns="http://www.w3.org/2000/svg" class="icon icon-twitter">
<use href="/icons/twitter.svg"></use>
</svg>

which simply doesn't work without the id and # in the href.

so i would propose that we switch to the icons.svg#<id> notation unless someone has a use version that works without touching the actual svg file.

@davidnuescheler
Copy link
Contributor

davidnuescheler commented Jul 31, 2020

after looking a little bit more into this and having usecases where it is pretty useful to use the same icons also for the favicons, i think that a project should be able to support both. icons in the /icons folder and icons that are in the icons.svg symbol sheet.

the easiest way to distinguish those would be to support :logo: and :#logo: notation respectively...

:logo: would product something like a <img class="icon icon-logo" src="/icons/logo.svg"> while :#logo: would produce <svg class="icon icon-logo"><use xlink:href="/icons.svg#logo"></use></svg>

@rofe
Copy link
Contributor Author

rofe commented Aug 3, 2020

I opened a new issue for this

@rofe rofe closed this as completed Aug 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request good first issue Good for newcomers released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants