Page MenuHomePhabricator

Store real SVGs icon files in the repo
Closed, DeclinedPublic

Description

Currently all icons are stored in code as SVG path strings: https://github.com/wikimedia/wvui/blob/master/src/themes/icons.ts

Icons are then built using a Vue template. This means that no SVG files exist in the repo, and therefore the process for editing/creating new icons is a lot more convoluted. Also many code review tools support SVG rendering, so if there were real files, icon changes could be seen in diffs. Icons could also be previewed in the file system, loaded into SVG editors easily, and exported to other codebases.

If it is a requirement that the current SVG template is enforced, a simple linter could verify this (although it could just be enforced by convention in the meantime, as we have done successfully in OOUI).


Additional hurdle is adding SVGs to the lib needs another transformational step to single path SVG objects, which is developer unfriendly for readability (diffing) reasons _and_ more work when adding/changing existing icons. Icon addition is a common task with the still smaller icon collection of Wikimedia Design Style Guide and will occur often (possibly 3-6 icons per quarter, not counting the variants)

Event Timeline

Another possibility could be to basic style assets, recent design systems summarize them as tokens and tokenized assets, in WikimediaUI Base with help of Style Dictionary.
This would make them also cleaner available for use cases beyond the library, like apps.

In last year, there were 22 different SVG path concerning patches for OOUI's icon collection, including 12 additions. From a maintenance perspective having real SVGs in the repo is a must.

Big+1 to having SVGs. Another problem with the path is that it limits the kind of SVGs you can ship. If we want to ship these as JS that's different, but I think if we do that, this code should make use of a JSON generated from the SVGs to make it more transparent which SVGs we are using (I'm thinking basic stuff like going to a URL to see the current SVG in the code repository).

Thanks @Jdlrobson, @AnneT was actually talking about a similar technical approach of auto-generating the single paths outputs we currently build on top. Anne pls correct me if I misunderstood your comment.
One thing unclear to me is if such a automatic transformation would work with higher complex SVGs as well, this could be a pitfall. On the lucky side, our icon guidelines ask for “simple and symmetrical geometric shapes” so we might be good..

@Volker_E that's right, although I'm also not sure we'll be able to successfully convert SVG contents to a single path every time, so we might want to see if we can extract all SVG paths and shapes (maybe just in a string?) then insert them into the icon component template directly, instead of setting a value to the path in that template. @santhosh recommended we check out how it's done in Material Design, or perhaps @Catrope has already whipped something up

I wrote a simple script that takes all svg icons in a directory and generate a JSON file in the format that we can use in Vue. This script when used with npm run build or run independently as and when needed abstracts the json and its use from designers.

I used SVGO, a library that we already use to optimize icons to convert all multi path svgs to single paths.

Demo application written using Vue and the generated json: https://wiki-icons.pages.dev/
Source code: https://github.com/santhoshtr/wiki-icons

I used all icons from the OOUI wikimedia themes. 959 files were processed. A few icons were found problematic for SVGO to convert to single path. See below:

npm run build:icons

> wiki-icons@0.1.0 build:icons
> node build-icons.js

Could not generate single path SVG for alignCenter.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><path d="M1 15h18v2H1zM1 3h18v2H1z"/><rect width="8" height="6" x="6" y="7" rx="1"/></svg>
Could not generate single path SVG for alignLeft.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><path d="M1 15h18v2H1zm11-8h7v2h-7zm0 4h7v2h-7zM1 3h18v2H1z"/><rect width="8" height="6" x="1" y="7" rx="1"/></svg>
Could not generate single path SVG for alignRight.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><path d="M1 15h18v2H1zm0-8h7v2H1zm0 4h7v2H1zm0-8h18v2H1z"/><rect width="8" height="6" x="11" y="7" rx="1"/></svg>
Could not generate single path SVG for feedback-ltr.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><path d="M19 16 2 12a3.83 3.83 0 0 1-1-2.5A3.83 3.83 0 0 1 2 7l17-4z"/><rect width="4" height="8" x="4" y="9" rx="2"/></svg>
Could not generate single path SVG for folderPlaceholder-ltr.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><path d="M8 2H2a2 2 0 0 0-2 2v2h12z"/><rect width="20" height="14" y="4" rx="2"/></svg>
Could not parse pause.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><rect width="6" height="16" x="3" y="2" rx="1"/><rect width="6" height="16" x="11" y="2" rx="1"/></svg>
Could not generate single path SVG for pause.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><rect width="6" height="16" x="3" y="2" rx="1"/><rect width="6" height="16" x="11" y="2" rx="1"/></svg>
Could not generate single path SVG for speechBubble-ltr.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><path d="M6 14H0v6z"/><rect width="20" height="16" rx="2"/></svg>
Could not generate single path SVG for speechBubbles-ltr.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><path d="M17 4v7a2 2 0 0 1-2 2H4v1a2 2 0 0 0 2 2h10l4 4V6a2 2 0 0 0-2-2zM6 10H0v6z"/><rect width="16" height="12" rx="2"/></svg>
Could not parse stop.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><rect width="16" height="16" x="2" y="2" rx="1"/></svg>
Could not generate single path SVG for stop.svg
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20"><rect width="16" height="16" x="2" y="2" rx="1"/></svg>

These icons seems to be simple enough to covert to single path. but SVGO does some checks on original svg before converting these shapes(only rect here) and bails out. These rectangles does not have ry value, which seems be an error as per the documentation of rx, and ry. May be we can fix these things in the 11 icons itself?

Does this look a reasonable solution?

Change 711028 had a related patch set uploaded (by Catrope; author: Catrope):

[wvui@master] [icon] Make icons real SVG files instead of path strings

https://gerrit.wikimedia.org/r/711028

I wrote a simple script that takes all svg icons in a directory and generate a JSON file in the format that we can use in Vue. This script when used with npm run build or run independently as and when needed abstracts the json and its use from designers.

Nice! I will take a look at this tomorrow. I had already written a patch on Friday that I just uploaded today (link above) that uses a Webpack plugin to import SVG files as strings. It also changes the way the icon component works to allow multiple SVG nodes rather than requiring a single path. But I may be able to avoid that by having SVGO convert everything to single paths the way you demonstrate in your script, that would make some things easier (and would allow me to avoid making any changes to the icon component). I'll look into that tomorrow.

great. Using SVGO for converting svg to single paths brings the original benefits of SVGO too- making SVGs clean and compact. Advantage of single path SVGs is, we are making our UI compatible with popular icon packs so that potentially wvui can be used with a different icon pack than the default one. For example, writing a mediawiki theme using WVUI with materialdesign icons. Or FontAwesome icons- They have a detailed document on preparing icons with single path. But to clarify, in the above approach, we are not asking designers to prepare it with single path, the script does that job. Even then guidelines to make icons simple helps at design stage.

After reading your patch, I think we can convert my script to a similar loader plugin, preferably with rollup since that is the direction we want to proceed(an example plugin). That brings the advantages of tighter coupling with devserver and HMR(icon changes get immediately seen on screen, but this usecase may be rare). We should be careful to narrow down the scope of this plugin to icons alone since loading real, complex, multi color svg assets to UI is a valid usecase and plugins should not interfere that.

After reading your patch, I think we can convert my script to a similar loader plugin,

I agree. I'll work on this some time this week or next (when depends on how busy I am with the summit and related things).

preferably with rollup since that is the direction we want to proceed(an example plugin).

Agreed. Thankfully the Webpack and Rollup plugin APIs are similar enough that converting from one to the other should be easy.

That brings the advantages of tighter coupling with devserver and HMR(icon changes get immediately seen on screen, but this usecase may be rare).

I agree that it's a rare use case, but it was certainly useful for me when I was rebuilding the icon system for my patch :) (which is not an everyday task of course)

We should be careful to narrow down the scope of this plugin to icons alone since loading real, complex, multi color svg assets to UI is a valid usecase and plugins should not interfere that.

That's a great point. My patch applies it to all .svg files, but it would be trivial to apply it only to .svg files in the icons/ directory, and we should clearly do that. It won't make much of a difference for WVUI or a similar component library repo where there probably won't be any SVG assets like that. But some repos with user-facing code (e.g. MediaWiki extensions) will need both custom icons and SVG assets. They should be able to easily reuse/copy this icon loading code without having to worry about breaking their SVG assets. That's a use case I hadn't thought about.

These icons seems to be simple enough to covert to single path. but SVGO does some checks on original svg before converting these shapes(only rect here) and bails out. These rectangles does not have ry value, which seems be an error as per the documentation of rx, and ry. May be we can fix these things in the 11 icons itself?

It turns out this is the opposite of the problem: SVGO's convertShapeToPath plugin only converts rectangles that do not have rx or ry set (i.e. rectangles without rounded corners). If either rx or ry is set, it bails and doesn't convert it. (Setting only rx is valid, it just means that ry implicitly has the same value.) There are also a three other icons that SVGO doesn't convert to a single path string because they use <use> (code.svg, settings.svg and tableMergeCells.svg).

Converting rectangles with rounded corners to paths is very doable (using arcs, see e.g. this code), and we could try to upstream that feature into SVGO. Expanding <use> tags seems a little trickier, and I'm not sure it'd be desirable, because <use> tags reduce duplication and make the SVG smaller.

Instead, I think I'm going to abandon making all SVGs single path, and instead change the icon system so that path strings are still accepted but full SVGs can be passed in wrapped in an object.

This has been accomplished in the meantime in Codex by @Catrope. No need to have this remaining open in the soon to be deprecated WVUI library.

Change 711028 abandoned by VolkerE:

[wvui@master] [icon] Make icons real SVG files instead of path strings

Reason:

Not in plan for WVUI anymore. See Codex for replacement implementation. https://doc.wikimedia.org/codex/main/icons/overview.html

https://gerrit.wikimedia.org/r/711028