SVG inlining

[jrit]

@vokal/web this defaults the build to inline SVGs if they are less than 8KB. You can disable this per SVG if you want by using the attribute data-inline-ignore which is documented here https://www.npmjs.com/package/web-resource-inliner

This only affects SVG <use> so if you are using <img> or already have SVGs embedded nothing will change. This actually fixes SVG support on IE up through 11 without using a JS polyfill, I was doing that and ran into a number of problems. This is also more inline with what GitHub is doing for SVGs as described here https://github.com/blog/2112-delivering-octicons-with-svg

The new grunt task could possibly be pulled into its own module later.

Fixes #51

[Tathanen]

Why not the index file?

[jrit]

good point

[dmaloneycalu]

the index file is moved from source to build in the copy task, including it here would still lead to it being overwritten when running a build

[Tathanen]

Does this need to be run after copy then?

[jrit]

I'll make adjustments as needed for index, it shouldn't be too complicated

[dmaloneycalu]

Probably. Something to consider is that ngtemplates also ignores the index file, so that would need to change as well. Either that or copy references the .inlined index file now.

[jrit]

index should work now

[dmaloneycalu]

No documentation added to the README.

I'm not sure I'm really for this on a global level. Has this been considered as an optional plugin for ngtemplates or as a separate task in maybe the packaging step?

[jrit]

ngtemplates can only use transforms that are synchronous but web-resource-inliner is only async, so I had to set it up as a separate task. Is there a reason you don't think this is a good idea?

[Tathanen]

I do think it's maybe a little weird to have the copy step hard-reference the index file in the "inlined svg files" directory, regardless of whether your project even includes svgs.

[dmaloneycalu]

@jrit I really think this should at best be included as part of the "package" step. It's an optimization for svgs that we won't use in all projects. I agree with @Tathanen on referencing a temporary directory for the copy task just feels weird.

I'm assuming that it's not possible to run this after ngtemplates due to the .html files now being .js strings?

[jrit]

I'm assuming that it's not possible to run this after ngtemplates due to the .html files now being .js strings?

that is correct that after ngtemplates runs it is all JS strings and impossible to do anything useful with.

this should at best be included as part of the "package" step. It's an optimization for svgs that we won't use in all projects

we are already running other SVG steps, this is an extension of those which are also global - I think removing the step would be optimizing for no real reason, the additional build time is neglibile

little weird to have the copy step hard-reference the index file in the "inlined svg files"

ngtemplates is the same, and both have always had hard-coded paths in them, I'm not really sure how this is any worse now that the path is just in a different file

[Tathanen]

It's not really the hard-coding that's weird to me, it's "pull from the SVG folder" when my site might not have SVGs in it at all, just a buncha weird SVG stuff in here running all the time with no effect.

It's largely a pedantic concern though, it seems to be perfectly functional as-is.

[jrit]

Yeah, it is possible that there is no effect. As an outcome of having a convention-based project setup that doesn't require much configuration on each project, there may be tasks that don't cause any changes. The alternative would be having more configuration at the project level, but we've generally been trying to avoid that. So ¯_(ツ)_/¯

[jrit]

Added new task to the readme.

[dmaloneycalu]

.inlined should be added to "Other Bits" in the readme as part of the .gitignore file. Also, I typo'd and am missing a 't' in "documentation" right above the "Other Bits" header if you could snag that as well.

[jrit]

thanks @dmaloneycalu - updated

[dmaloneycalu]

I think the watch tasks may be goofed. This inlining should be part of the templates task instead of the sprite task. Otherwise html changes won't trigger the templates being generated.

If I'm mistaken let me know, but svg_sprite only watches the image directory and ngtemplates now watches the .inlined directory

[jrit]

It should be both now. When SVGs change, inline and build templates. When templates change, inline and build templates, so the end result is always the same.

[dmaloneycalu]

These lines here need changed, otherwise saving a template file doesn't do anything. They should reference svg_inline's cwd, and src

[jrit]

Finally got around the working against this again and this seems to be working properly now.

[dmaloneycalu]

Needs a changelog update. After that I'm not seeing anything missing. Have @Tathanen confirm though. My vote is that this is still more of a feature change than a breaking change but that may be the outlier opinion for us.

[Tathanen]

LGTM after changelog update, yeah. I could maybe make an argument for a bit more elaboration in the svg_inline part of the README, not just saying "it inlines them" but why it inlines them, what it's accomplishing, why it's good. Just another sentence or two, similar to your opening PR post.

[jrit]

updated the docs, look good?

[Tathanen]

Inline joke, LBTM.

Add "[Breaking Changes]" after the version number in the changelog like with all the other majors, then should be good to go.