Directly embed icons into the icon-vars css file by juliusknorr · Pull Request #12016 · nextcloud/server

juliusknorr · 2018-10-24T13:27:26Z

This is a PoC to directly embed SVG icons into the generated CSS files. While it will have some influence on the loading time of the first request (since every svg file is read then by the server) it drastically reduces requests and loading time later on.

The icons are added base64 encoded to the icon-vars.css.

I did some quick&rough measurements on a quite suboptimal setup (no redis, sqlite, modphp)

Before/After

First loading
- Diabled browser cache 20s/20s
SCSS has been cached
- Disabled browser cache 15s/5s
- Enabled browser cache 3s/1.5s

Part of #1066

For testing you will need to run occ maintenance:repair to clear the scss cache.

Feedback is welcome. The code could need another cleanup before merging 🙈

cc @nextcloud/designers

MorrisJobke · 2018-10-24T13:57:54Z

+			$svg = preg_replace('/stroke="#([a-z0-9]{3,6})"/mi', 'stroke="#' . $color . '"', $svg);
+			$svg = preg_replace('/fill="#([a-z0-9]{3,6})"/mi', 'fill="#' . $color . '"', $svg);
+
+			$encode = base64_encode($file);


Or should $file be $svg?

juliusknorr · 2018-10-24T15:02:08Z

@MorrisJobke Can you try again? Seems the regex needed an adjustment as well 🙈

juliusknorr · 2018-10-24T15:02:52Z

Fix accessibility dark theme icons

MorrisJobke · 2018-10-24T15:52:22Z

Chrome somehow shows the old assets

Cleared caches again and now it works also in chrome.

rullzer · 2018-10-24T16:16:06Z

Nice work.
Doing this makes me think. More and more we should start shipping pregenerated css.

skjnldsv · 2018-10-24T16:40:28Z

It's very nice, but this goes directly against the idea of having a colour api, no? 🤷‍♀️
We don't fetch the data from the svg colour anymore, then? 🤔

juliusknorr · 2018-10-24T18:53:28Z

Doing this makes me think. More and more we should start shipping pregenerated css.

I would totally support that. The main issue why we included all the SCSS compilation logic was, to lower the entry barrier. But now that we have more and more vue apps, we require a npm based dev environment anyway, so we could move towards a preprocessed css as well.

It's very nice, but this goes directly against the idea of having a colour api, no? woman_shrugging
We don't fetch the data from the svg colour anymore, then? thinking

Yes, when I started with this I thought about keeping the api for the accessibility app, but that one could of course also just replace the colors in the inline images then.

skjnldsv · 2018-10-24T21:14:22Z

Yes, when I started with this I thought about keeping the api for the accessibility app, but that one could of course also just replace the colors in the inline images then.

I'm totally fine with the approach! But I would like to keep a way to still get colorized svg the way we do this :)

juliusknorr · 2018-10-25T08:10:37Z

I'm totally fine with the approach! But I would like to keep a way to still get colorized svg the way we do this :)

@skjnldsv You mean to keep the svg endpoint, or just a way to change the svg color?

skjnldsv · 2018-10-25T08:59:58Z

@skjnldsv You mean to keep the svg endpoint, or just a way to change the svg color?

Well both of them, having a way to sill colorized svg on the fly is very nice for devs I think? :)

Signed-off-by: Julius Härtl <[email protected]>

juliusknorr · 2018-10-25T10:26:47Z

Ready for review. The accessibility app still uses the svg api endpoint for now, since we don't cache the user css files on the server, this would increase the loading time with empty browser cache quite a bit. We still can do this in a follow up PR.

The flow of icons is now:

A scss file is compiled
The IconsCacher looks for --icon variables
- it creates a icons-list.template file with the original links to the svg api for later processing
- it creates a icons-vars.css file with the icons embedded as data url based on the icons-list.template
The accessibility app now also uses the icons-list.template to replace colors for inverted icons

juliusknorr · 2018-10-25T12:05:17Z

@MorrisJobke Can you check if that also fixes the rerendering issue on safari?

MorrisJobke · 2018-10-25T12:12:42Z

@MorrisJobke Can you check if that also fixes the rerendering issue on safari?

On it already ;)

skjnldsv · 2018-10-25T12:22:07Z

@juliushaertl So just so I get this right, we can't directly get svg coloured anymore, BUT if we declare them via css it will work?

MorrisJobke · 2018-10-25T12:23:48Z

@MorrisJobke Can you check if that also fixes the rerendering issue on safari?

Still re-renders everything. Only if the support icon is deleted from the page the re-rendering stops. 😢

skjnldsv · 2018-10-25T12:30:38Z

@MorrisJobke Can you check if that also fixes the rerendering issue on safari?

Still re-renders everything. Only if the support icon is deleted from the page the re-rendering stops.

Yes, but still a lot better without the requests in the background!

juliusknorr · 2018-10-25T12:40:58Z

So just so I get this right, we can't directly get svg coloured anymore, BUT if we declare them via css it will work?

Yes, All the mixins we have that use the icon-color mixin, will automatically be inlined. But fetching them from the svg endpoint would still work, in case apps use that.

skjnldsv · 2018-10-25T12:42:18Z

You rocks @juliushaertl !! 🎸 🎉

MorrisJobke

Works over here 👍

MorrisJobke · 2018-10-25T13:03:27Z

@juliushaertl Could you check the acceptance tests that fail? They look weird.

MorrisJobke · 2018-10-25T13:03:47Z

I also triggered a re-run: https://drone.nextcloud.com/nextcloud/server/11850

juliusknorr · 2018-10-25T13:57:40Z

@MorrisJobke They run fine locally for me. files and user acceptance tests indrone.nextcloud.com/nextcloud/server/11850 timed out again it seems 😢

jancborchardt · 2018-10-31T10:27:37Z

Wow @juliushaertl! Performance work always deserves praise! 🥇 :D

juliusknorr added enhancement 2. developing Work in progress labels Oct 24, 2018

juliusknorr added this to the Nextcloud 15 milestone Oct 24, 2018

juliusknorr requested review from MorrisJobke, pixelipo, rullzer and skjnldsv October 24, 2018 13:27

This comment has been minimized.

Sign in to view

MorrisJobke reviewed Oct 24, 2018

View reviewed changes

Comment thread lib/private/Template/IconsCacher.php Outdated

MorrisJobke reviewed Oct 24, 2018

View reviewed changes

Comment thread lib/private/Template/IconsCacher.php Outdated

MorrisJobke reviewed Oct 24, 2018

View reviewed changes

juliusknorr force-pushed the wip/noid/icon-base64 branch from e548dee to 5e166f2 Compare October 24, 2018 14:29

This comment has been minimized.

Sign in to view

juliusknorr added 3 commits October 25, 2018 11:58

Directly embed icons into the icon-vars css file

845f1b6

Signed-off-by: Julius Härtl <[email protected]>

Properly track data urls as well

152fb08

Signed-off-by: Julius Härtl <[email protected]>

Remove default which didn't work

0b2ef7e

Signed-off-by: Julius Härtl <[email protected]>

juliusknorr force-pushed the wip/noid/icon-base64 branch from 32ec824 to 390691d Compare October 25, 2018 09:58

Keep list of icons in a separate file for use in the accessibility app

d21ded6

Signed-off-by: Julius Härtl <[email protected]>

juliusknorr force-pushed the wip/noid/icon-base64 branch from 390691d to d21ded6 Compare October 25, 2018 10:16

skjnldsv approved these changes Oct 25, 2018

View reviewed changes

juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 25, 2018

MorrisJobke approved these changes Oct 25, 2018

View reviewed changes

MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 25, 2018

MorrisJobke merged commit 6ad7f32 into master Oct 25, 2018

MorrisJobke deleted the wip/noid/icon-base64 branch October 25, 2018 14:04

juliusknorr mentioned this pull request Oct 26, 2018

Optimize load times of images & icons #1066

Closed

kyrofa mentioned this pull request Dec 18, 2018

Server static resources directly via webserver #12290

Closed

Conversation

juliusknorr commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before/After

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

MorrisJobke Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Oct 24, 2018

Uh oh!

juliusknorr commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

MorrisJobke commented Oct 24, 2018

Uh oh!

rullzer commented Oct 24, 2018

Uh oh!

skjnldsv commented Oct 24, 2018

Uh oh!

juliusknorr commented Oct 24, 2018

Uh oh!

skjnldsv commented Oct 24, 2018

Uh oh!

juliusknorr commented Oct 25, 2018

Uh oh!

skjnldsv commented Oct 25, 2018

Uh oh!

juliusknorr commented Oct 25, 2018

Uh oh!

juliusknorr commented Oct 25, 2018

Uh oh!

MorrisJobke commented Oct 25, 2018

Uh oh!

skjnldsv commented Oct 25, 2018

Uh oh!

MorrisJobke commented Oct 25, 2018

Uh oh!

skjnldsv commented Oct 25, 2018

Uh oh!

juliusknorr commented Oct 25, 2018

Uh oh!

skjnldsv commented Oct 25, 2018

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Oct 25, 2018

Uh oh!

MorrisJobke commented Oct 25, 2018

Uh oh!

juliusknorr commented Oct 25, 2018

Uh oh!

jancborchardt commented Oct 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

juliusknorr commented Oct 24, 2018 •

edited

Loading

juliusknorr commented Oct 24, 2018 •

edited

Loading