-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Directly embed icons into the icon-vars css file #12016
Conversation
This comment has been minimized.
This comment has been minimized.
lib/private/Template/IconsCacher.php
Outdated
$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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should $file
be $svg
?
e548dee
to
5e166f2
Compare
@MorrisJobke Can you try again? Seems the regex needed an adjustment as well 🙈 |
|
This comment has been minimized.
This comment has been minimized.
Cleared caches again and now it works also in chrome. |
Nice work. |
It's very nice, but this goes directly against the idea of having a colour api, no? 🤷♀️ |
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.
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 :) |
@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 <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
32ec824
to
390691d
Compare
Signed-off-by: Julius Härtl <jus@bitgrid.net>
390691d
to
d21ded6
Compare
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:
|
@MorrisJobke Can you check if that also fixes the rerendering issue on safari? |
On it already ;) |
@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? |
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! |
Yes, All the mixins we have that use the |
You rocks @juliushaertl !! 🎸 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works over here 👍
@juliushaertl Could you check the acceptance tests that fail? They look weird. |
I also triggered a re-run: https://drone.nextcloud.com/nextcloud/server/11850 |
@MorrisJobke They run fine locally for me. files and user acceptance tests indrone.nextcloud.com/nextcloud/server/11850 timed out again it seems 😢 |
Wow @juliushaertl! Performance work always deserves praise! 🥇 :D |
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
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