added icon plugin

[wkeese]

According to the API doc:

This plugins loads SVG files and merges them into one SVG sprite

When is this plugin supposed to be used? Is it part of a build step, rather than being used when the application runs?

[tkrugg]

I was conveniently burying the complexity in that one sentence but you're right, it should be more specific.

You start by writing this in your application

define([
    "requirejs-dplugins/icon!./icon1.svg",
    "requirejs-dplugins/icon!./icon2.svg"
], function(){...})

The code of this plugins runs in 3 situations

1. build step ran by grunt-amd-plugin it will create a sprite.svg that basically concatenates icon1 and icon2
2. runtime of application that isn't buit it's fetching icons separately and builts the sprite on the client. You might think creating a sprite in the browser when all icons have already been fetched is useless, but if an icon appears 10 times in the same page, it helps keeping the dom lighter.
3. runtime of application that is buit with grunt-amd-build it fetching the sprite.svg instead of icon1.svg and icon2.svg

[wkeese]

build step ... will create a sprite.svg that basically concatenates icon1 and icon2

OK, that makes it clearer. But why create a sprite.svg file rather than just inlining the SVG text into the layer's JS files, the same way as the text! and IIRC the handlebars! plugins work?

runtime of application that isn't built ... builds the sprite on the client. You might think creating a sprite in the browser when all icons have already been fetched is useless, but if an icon appears 10 times in the same page, it helps keeping the dom lighter.

That went completely over my head. Is this about not repeating the same block of SVG on the page 10 times?

[tkrugg]

... But why create a sprite.svg file rather than just inlining the SVG text into the layer's JS files, the same way as the text! and IIRC the handlebars! plugins work?

It seemed to me the only reason we went for wrapping templates in js module is so that we can concatenate them during the build then fetch them all in one file, and html offers no other way to do it. SVG does so I went for it.

Is this about not repeating the same block of SVG on the page 10 times?

The SVG markup of some simple icons can get very big (example), the fact that the sprite is built using <symbol> makes it easy to use your icon with a simple <use> tag, but you could imagine a <d-icon> widget that would take the markup of that symbol and inline it.

[wkeese]

IIUC, at runtime your plugin loads the icons (or sprite) using the requirejs-text! plugin. If that's true, I don't see any reason to reinvent the wheel by building a new system for creating layer files. You've got two pages of code for doing builds (and for being aware of the builds at runtime) whereas delite/handlebars! only has a few lines of code.

I understand how the <use> tag could avoid bloating the DOM when an icon is used multiple times, but I don't think you need a sprite to use <use>. For example, https://developer.mozilla.org/en-US/docs/Web/SVG/Element/use shows how an app can define a single circle "icon" and then reference it multiple times with the <use> tag. (So the code in common.js also seems suspect.)

Relatedly, the tests you've written are all checking the sprite, which is sort-of an implementation detail. You should have basic tests from an end-user perspective, i.e. test that the icon is available after the plugin finishes loading.

The other thing, which I think I've mentioned before, is that if this plugin and the <d-icon> element are to be used together, they should be in the same project. Will anyone use this plugin standalone?

[cjolif]

The other thing, which I think I've mentioned before, is that if this plugin and the element are to be used together, they should be in the same project. Will anyone use this plugin standalone?

Yes. We plan to use it in other contexts.

[clmath]

I think having pure svg layer give additional value since it would allow someone to load the svg without requirejs. So it is a more agnostic solution than inlining the svg directly in the requirejs-specific layer.

But I agree with you that there is some duplication in the build code for the various plugins. When I get some time I will try to refactor this in a shared module providing build utilities.

Regarding the tests, I think testing the structure of the sprite is enough. The actual binding using <use> is done by the browser and I don't think we want to start testing the browser

@tkrugg A sample use of the plugin would be welcome ;)

[wkeese]

I think having pure svg layer give additional value since it would allow someone to load the svg without requirejs.

That sounds like a stretch. Our whole architecture and even this plugin itself is based on AMD.

This design is adding pages and pages of code with no regression test.

Regarding the tests, I think testing the structure of the sprite is enough. The actual binding using is done by the browser and I don't think we want to start testing the browser.

And I say, don't test the structure of the sprite at all, because that's an implementation detail. Just test that the icons get loaded.

[clmath]

What do you propose to check that the icon loads ? I have not been able to come up with a better way than testing the sprite. Also I don't think the sprite is an implementation detail but the main output of the plugin.

According to the plugin documentation:

This plugin load svg icons and declares them in one sprite, insert inline in the DOM, so that you can reference them in a <use> tag.

So I would rather see the sprite as the interface for the user to load an icon than an implemention detail.

[wkeese]

What do you propose to check that the icon loads ? I have not been able to come up with a better way than testing the sprite.

Well, the icon.md documentation says that the user is supposed to do <use xlink:href="#icon1"></use> after the plugin returns. So you want to confirm that works. If it's too hard to directly test <use> tags, then probably you can do document.querySelectorAll("#icon1").

So I would rather see the sprite as the interface for the user to load an icon

I'm not sure what you mean. The interface to load the icon is define([ "requirejs-dplugins/icon!./icon1.svg" ... and the interface to use the icon is <use xlink:href="#icon1">. This is how it's documented in icon.md.

One thing that's unclear is what href to use. Is it based on the file name? That seems weak, because you could easily have two icon files with the same name. I would expect instead this interface:

define([
    "requirejs-dplugins/icon!./icon1.svg",
    "requirejs-dplugins/icon!./icon2.svg"
], function(href1, href2){
    // use href1 and href2 to generate your <use> tags
    ...

Side note: icon seems like a bad name for this plugin. The plugin can load any SVG, not just icons, and conversely, icons are usually image files rather than SVG. So svg is probably a better name for the plugin.

[wkeese]

I think something like this makes sense: https://github.com/wkeese/requirejs-dplugins/blob/3de62c53ce3feb7a2ed2855677e0a07049bc4814/icon.js.

It's less than half the size of the original code, and will also load quicker because there's one less network request. OTOH, if you stick with the current design, then you need to add a lot more tests, such as when there are multiple sprites, and when there's a sprite but also some icons are loaded individually.

[tkrugg]

I like your idea of leveraging the text plugin, I wasn't aware of it, and it is impressively lowering the size of the plugin. Thanks for taking the time to explain and implement what you meant, I really appreciate.

I can think of a few functional differences:

• your build isn't ending up with an actual svg sprite, but rather a js file that can't be optimized/simplified through the usual tools. But that isn't really an issue, most optimisations can be done directly on the icons.
• the js sprite stores the icons as fetched, with all that extra markup you need for a standalone icon but potentially have no use for in a sprite (I'm thinking doctype and various attributes symbols such as width, height, xmlns...). The resulting sprite would be a little bigger.
• when running the built app, you're fetching one js file, saving bandwidth, but you're still doing markup parsing for each of the n icons in it, and injecting them in the DOM one by one. I'm not sure whether this extra load compensates for the additional lines I currently have. Not mention theses extra chunks of code are essentially wrapped under an if (has("builder")) block, and should vanish in production code.

[wkeese]

That's a good analysis. I agree there are advantages to do parsing at build time...

The <!DOCTYPE etc. ideally shouldn't be sent across the wire at all. Sending it multiple times is even worse, although gzip probably takes care of most of the repeated text. But could you have a write() method used during builds that parses out the <symbol>...</symbol> and adds only that text to the JS layer, i.e. a variation on how the text! plugin works? You wouldn't be leveraging the text! plugin anymore but at least it would allow a single code-path (for the most part) at run time.

Speaking of parsing, the current _innerSVG() method is a bit crazy. You end up parsing the SVG, then converting it back to text, with a bunch of clone() calls, and then parsing the text again. It would be better to just parse once, and then do something like:

svgSprite.appendChild(iconFileDOM.querySelector("symbol"));

theses extra chunks of code are essentially wrapped under an if (has("builder")) block, and should vanish in production code.

That's true for some of the code, but not all, in particular code in common.js. BTW, it seems like you shouldn't have a separate file called common.js since it's only used by icon.js.

[wkeese]

PS: What I wrote above about _innerSVG() didn't quite make sense, but it does seem possible to remove the method. See https://github.com/wkeese/requirejs-dplugins/blob/tkrugg-icon/icon.js. It just parses the SVG once and then adds it to the document.

I also noticed that all your example icons have a single <g> root node. If that's always the case then seems like you don't need to create a <symbol> wrapper node at all. But probably it's legal for <svg> to have multiple children? If so you should have that test case too.

[tkrugg]

I've done some changes but i've put them in another branch https://github.com/tkrugg/requirejs-dplugins/pull/1. i'll merge it into this one eventually.

[wkeese]

(copying my comments from the other PR...)

Thanks for the simplifications. The definitely makes it easier to compare approaches, and I like how it's sharing more code than before.

One thing though: at runtime, it makes sense to call extractGraphic() and createSymbol() on raw icon files, but IIUC you are also calling those functions on pre-built sprites, which already contain <symbol> tags. That seems strange.

Relatedly, I'm unclear what happens when you load (for example) two pre-built sprites (each containing 10 icons) and two stand-alone icon files. What DOM do you end up with? A single <svg style=display:none> node that contains all 22 icons? Do you end up with <symbol> tags nested inside other <symbol> tags or something weird like that?

Minor note: Might want to combine extractGraphic() and createSymbol(), since they are always called one right after the other. Relatedly, the comment describing extractGraphic() isn't quite right.

[tkrugg]

IIUC you are also calling those functions on pre-built sprites, which already contain tags. That seems strange.

Yes the sprite is loaded as one big icon. For example if the <layer>.min.js file holds 3 icons, the markup in the file should look somthing like this:

<svg>
    <symbol id=icon1..>
    <symbol id=icon2..>
    <symbol id=icon3..>
</svg>

And this svg file can be loaded like any icon and the markup in the browser will look like:

<svg style="display:none">
    <symbol id="layer.min">
       <symbol id=icon1..>
       <symbol id=icon2..>
       <symbol id=icon3..>
   </symbol>
</svg>

Note that this isn't weird at all. It's totally normal to combine symbols to create other symbols. According to the spec, it's a structural element that can contain any number of other structural elements.

• This is a simplification because it's hard to guess whether what we've loaded is a sprite or isn't, also a simplification as it comes down to using the same code from both the sprite and the icon
• This a feature because a user of the plugin might want to use his/her own sprite with another level of nesting (not generated with this plugin), or load an icon that's defining symbols used internally...

[wkeese]

It's totally normal to combine symbols to create other symbols.

I see, that's a reasonable argument.

One issue I notice is that svg!icon.svg will normally evaluate to icon.svg (i.e require(["svg!icon1.svg"], function(id){ console.log(id); }) will print icon.svg), except when icon1.svg is inside a sprite. At that point it evaluates to sprite.svg or something like that. Shouldn't it still evaluate to icon.svg?

That reminds me about the problem I mentioned earlier about when two icons have the same id because the have the same filename, for example deliteful/Combobox/arrow.svg and deliteful/DropDownButton/arrow.svg. It would be nice to resolve this somehow.

A related consideration is the corner case when the app sets up a AMD map to (for example) load two separate versions of deliteful/Combobox, with corresponding icons d1/Combobox/arrow.svg and d2/Combobox/arrow.svg.

[clmath]

One issue I notice is that svg!icon.svg will normally evaluate to icon.svg (i.e require(["svg!icon1.svg"], function(id){ console.log(id); }) will print icon.svg), except when icon1.svg is inside a sprite. At that point it evaluates to sprite.svg or something like that. Shouldn't it still evaluate to icon.svg?

I think the plugin should return the id of the svg graphic requested. This may be the file name of the svg or the content of the id attribute on the symbol element. The tricky part is to keep the binding between requested file name and icon id when we load the sprite.

That reminds me about the problem I mentioned earlier about when two icons have the same id because the have the same filename, for example deliteful/Combobox/arrow.svg and deliteful/DropDownButton/arrow.svg. It would be nice to resolve this somehow.

I don't think this is an issue as it can be solved by using different ids in the svg markup.

A related consideration is the corner case when the app sets up a AMD map to (for example) load two separate versions of deliteful/Combobox, with corresponding icons d1/Combobox/arrow.svg and d2/Combobox/arrow.svg.

I don't see a work-around for this one, but I am not sure there is much we can do about it. There would be the same issue if the two Combobox use the same CSS class names with different implementation. Since all CSS is gloabal one stylesheet would override the other.

Since we have come to minor issues and we need to make progress here, I am merging this in e76c16d874807c00529800bd22af1934a7c943e5 and opening defect #29 to track the issue with returned value when loading sprites.

[wkeese]

I'm disappointed you checked this in. As I mentioned already I question the whole design with generating a separate sprite file. It leads to many complications.

[wkeese]

I don't think this is an issue as it can be solved by using different ids in the svg markup.

What if there are two separate packages from separate authors that both have a file called arrow.svg? For example, like dijit and idx. And then there's some app using both of those packages?

This is the basic concept about why global variables are bad, and relatedly, why we have namespaces (for example java.lang.xyz) or AMD paths (example deliteful/Combobox) rather than just global names like xyz and Combobox.

[wkeese]

Since you already closed this ticket, I filed #30 for the problem with duplicate ids. In that ticket I explained why adding explicit id's to icon files doesn't solve the problem, and instead, list multiple ways that you can solve the problem.

From your actions, it's clear that you are rushing to start using this plugin before it's finished, but that's a bad idea, because the API is going to change. Specifically, you need to get the icon's id from the return value of the svg! plugin rather than assuming that the id matches the filename. At a minimum that means that #29 needs to be fixed.