create an svg icon sprite for all action icons

[ChristophWurst]

This should decrease the number of requests we need for (initial) page loads, ref ref #2272

doc https://github.com/jkphl/svg-sprite/blob/master/docs/configuration.md
Fixes https://github.com/jkphl/svg-sprite/blob/master/docs/configuration.md

Ideally, we should do this step-by-step because this change could potentially break stuff, especially in third-party apps. Therefore I will limit this PR to the basic setup (Grunt+config) and one example, the action icons. Other icons can be converted in follow-up PRs.

---

Follow-up idea/PR: use grunt+uglify to combine vendor scripts which are loaded on every page (jQuery, jQuery migrate, jQuery ui, Backbone, es6promise, …).

cc @nextcloud/designers
fixes #1066

[mention-bot]

@ChristophWurst, thanks for your PR! By analyzing the history of the files in this pull request, we identified @skjnldsv, @rullzer and @MorrisJobke to be potential reviewers.

[skjnldsv]

Couldn't we generate it?

[ChristophWurst]

Couldn't we generate it?

Generate what? :-)

[skjnldsv]

The svg sprite! From the original sprites! ^^
I don't know if that was what you implied 😆

[MorrisJobke]

I would really love to have a make file for this, that takes care of installing grunt and calling the right binaries, etc ... I don't want to fiddle around how to install it and which version and what the command then is.

[ChristophWurst]

I can add one ofc

[ChristophWurst]

@skjnldsv yes, this PR is about combining multiple svg icons into an image sprite to speed up our front-end a bit :-)

[nickvergessen]

Hmm but we can't really drop the original svgs as they are used by other apps (e.g. activity/notifications) and are even linked to, so apps can use them. For these cases we need the "single icon per file" version as a fallback anyway.

But should help to reduce the number of requests until one of the apps is used.

[skjnldsv]

One more issue though is the background. Using a sprite as an icon with a recurrent width is easy, but since we use it for various elements tat sometimes are very large , we need to have each sprite very far from each others :)

[ChristophWurst]

One more issue though is the background. Using a sprite as an icon with a recurrent width is easy, but since we use it for various elements tat sometimes are very large , we need to have each sprite very far from each others :)

I think that's probably the source of most of the problems I have found so far (see the list above).

[skjnldsv]

Well, to be fair, the number of svg by px² shouldn't change the weight of the file.
And our goal is the number of requests, not as important of the weight imho :)

[nickvergessen]

We can also have a look towards HTTP2 instead of this. Then the number of requests dont matter that much anymore...

[skjnldsv]

We could also go for base64 encoded svg inside the compiled scss :)

[ChristophWurst]

We can also have a look towards HTTP2 instead of this. Then the number of requests dont matter that much anymore...

In which regard would HTTPS2 help here?

[skjnldsv]

Pipelining? Will send all the data all-at-once avoiding the cost of using sprites.

[ChristophWurst]

@nickvergessen @skjnldsv did you try nc with HTTPS2? I gave it a test run a few months ago and I could not really see the huge benefit. I think we still load far too many files (~200 request per page load).

Related: http://stackoverflow.com/questions/28630108/does-minifying-and-concatenating-js-css-files-and-using-sprites-for-images-stil#28631462

[nickvergessen]

I could not really see the huge benefit

Well yes, because we don't use it yet.

[ChristophWurst]

Well yes, because we don't use it yet.

How can we be not using that HTTP2 feature? It's nothing the site would have to enable, would it?

Edit: In any case, this still is a big problem. I just tested it again with nginx+TLS+http2, immutable cache and FF: it takes the browser a few seconds to even load those many assets from cache, you can track it in the network tab. Therefore, I am convinced that HTTP2 will not magically solve the performance problems we're facing on the client side, unfortunately.

[nickvergessen]

Well we can add async attributes to JS and css files, etc. I didn't look too much into it.

[ChristophWurst]

Well, to be fair, the number of svg by px² shouldn't change the weight of the file.
And our goal is the number of requests, not as important of the weight imho :)

I've added 100px spacing in between the individual SVGs following the discussion at svg-sprite/svg-sprite#200 (comment).

[ChristophWurst]

Okay, so now with the correct spacing less stuff looks broken, although still many icons are either not shown at all (e.g. the new folder icon in the files app) or are misplaced (e.g. the import button in calendar where the background position is manipulated by the app).

[jancborchardt]

@ChristophWurst Do we have any issues or questions relating to the SVG sprite tool? Then let’s work with @jkphl to fix it :)

[ChristophWurst]

@jancborchardt so far it seems to work. The problem here is our existing css 😉

[jkphl]

@jancborchardt @ChristophWurst Unfortunately, my spare time is very limited these days, but if you guys need help with svg-sprite please let me know and I'll do my best.

[MorrisJobke]

@jancborchardt so far it seems to work. The problem here is our existing css 😉

Okay ... then we maybe look into fixing this and go for the sprite approach. No need to delay it further. 😉 I want to get this in quite fast to have more potential testers :)

[ChristophWurst]

Fixed yet another issue with icons in popover menus. I'm not sure if I missed something, but this looks almost good to me. All the remaining issues seem to be inside apps, mostly because they overwrite the background position for stuff like the new mail/contact button:

cc @nextcloud/calendar @nextcloud/contacts @nextcloud/gallery

[codecov-io]

Codecov Report

Merging #3385 into master will increase coverage by 0.18%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##             master    #3385      +/-   ##
============================================
+ Coverage     54.28%   54.47%   +0.18%     
- Complexity    21061    21323     +262     
============================================
  Files          1310     1311       +1     
  Lines         80354    81931    +1577     
  Branches       1250     1367     +117     
============================================
+ Hits          43621    44629    +1008     
- Misses        36733    37302     +569

Impacted Files	Coverage Δ	Complexity Δ
core/templates/layout.user.php	0% <0%> (ø)	0 <0> (ø)	❌
lib/private/legacy/template.php	45.02% <100%> (+0.32%)	35 <0> (ø)	❌
lib/private/Setup.php	11.55% <0%> (-1.46%)	59% <0%> (+10%)
settings/js/apps.js	27.88% <0%> (-0.79%)	0% <0%> (ø)
config/config.sample.php	0% <0%> (ø)	0% <0%> (ø)	❌
lib/private/Template/JSConfigHelper.php	0% <0%> (ø)	16% <0%> (ø)	❌
lib/private/TemplateLayout.php	0% <0%> (ø)	38% <0%> (+6%)	✅
ocs/v1.php	0% <0%> (ø)	0% <0%> (ø)	❌
core/register_command.php	0% <0%> (ø)	0% <0%> (ø)	❌
public.php	0% <0%> (ø)	0% <0%> (ø)	❌
... and 19 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d754227...8f3e5f7. Read the comment docs.

[skjnldsv]

This was used to cancel the min height/width applied to the icons. Since we're using padding to define the height, we don't want the 16x16px minimum set on [class^='icon-'], [class*=' icon-']

[ChristophWurst]

I don't really know how to properly implement this without breaking too much. If someone wants to take over, please do so, otherwise I would close this.

[MorrisJobke]

Alternative approach: #4059

[rullzer]

Lets close this as it breaks to much. It should be cached anyways.

[rullzer]

Feel free to reopen when somebody feels motivated.

[jancborchardt]

Referencing the issue about it: #1066 just for the record :)