Svg color api by skjnldsv · Pull Request #9984 · nextcloud/server

skjnldsv · 2018-06-25T09:20:50Z

This create a new endpoint to get one of our svg and change the colour.
This will allow us to:

Only maintain black svg
Only maintain one set of icon
Allow us not to use any filter to invert icons: ie11 compatibility and better handling of background images

@nextcloud/designers do you see any issues with this?
@juliushaertl how do you think we should handle custom paths? Right now I implemented this for the actions folder only, but in the end we'll probably need this for the apps menu and other stuff.

Any example on where to use this?

MorrisJobke · 2018-06-25T09:33:10Z

core/Controller/SvgController.php

+	 * @return DataDisplayResponse|NotFoundException
+	 */
+	public function getSvg(string $fileName, $color = 'ffffff') {
+		$path = $this->serverRoot . "/core/img/actions/$fileName.svg";


This then needs to be redone for all the other icon locations :/

Yeah, it was a first shot, I'm asking for feedback on how to tackle this :)

how do you think we should handle custom paths? Right now I implemented this for the actions folder only, but in the end we'll probably need this for the apps menu and other stuff.

On the other side I'm fine with doing it like this for now and test it.

Lets first try it like this. I'd rather have 3 endpoints that are dead simple. (and do their own stuff) than 1 endpoint that does a lot of stuff.

@rullzer though we'll need to access icons in apps, so I guess I will add a new endpoint for apps icons

Sure but like I said. Small endpoints beat complex ones. so 🚀

jancborchardt · 2018-06-25T13:24:41Z

core/css/icons.scss

 	&:hover,
 	&:focus {
-		background-image: url('../img/actions/delete-hover.svg?v=1');
+		background-image: url('#{$webroot}/svg/icon-delete/d40000?v=1');


icon-star (or icon-starred) then also needs the yellow star color. And the folders the Nextcloud blue (or rather theming color)?

@jancborchardt that's correct, we can extend this to whatever colours we want :)

juliusknorr · 2018-06-26T08:21:50Z

how do you think we should handle custom paths? Right now I implemented this for the actions folder only, but in the end we'll probably need this for the apps menu and other stuff.

I think we can add another endpoint that is a bit more generic, so it works for other apps with different icon paths as well. We do have such a route in the theming app already: https://github.com/nextcloud/server/blob/master/apps/theming/appinfo/routes.php#L80-L84

skjnldsv · 2018-06-26T09:59:08Z

I've added a new /svg/app/icon/color path :)

juliusknorr · 2018-06-29T07:15:35Z

core/Controller/SvgController.php

+
+		// Set cache control
+		$ttl = 31536000;
+		$response->addHeader('Cache-Control', 'max-age=' . $ttl . ', immutable');


Use $response->cacheFor() https://docs.nextcloud.com/server/13/developer_manual/api/OCP/AppFramework/Http/Response.html#OCP\AppFramework\Http\Response::cacheFor

juliusknorr · 2018-06-29T07:17:13Z

I've added a new /svg/app/icon/color path :)

@skjnldsv I cannot find it, maybe you forgot to push? 😉

skjnldsv · 2018-06-29T07:20:33Z

@skjnldsv I cannot find it, maybe you forgot to push? wink

😅😅

codecov · 2018-06-29T11:15:52Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a788f49). Click here to learn what that means.
The diff coverage is 3.57%.

@@            Coverage Diff            @@
##             master    #9984   +/-   ##
=========================================
  Coverage          ?   52.06%           
  Complexity        ?    25950           
=========================================
  Files             ?     1646           
  Lines             ?    95867           
  Branches          ?     1290           
=========================================
  Hits              ?    49910           
  Misses            ?    45957           
  Partials          ?        0

Impacted Files	Coverage Δ	Complexity Δ
core/routes.php	`0% <ø> (ø)`	`0 <0> (?)`
core/Controller/JsController.php	`93.54% <ø> (ø)`	`7 <0> (?)`
core/Controller/SvgController.php	`0% <0%> (ø)`	`4 <4> (?)`
lib/private/Template/SCSSCacher.php	`72.48% <100%> (ø)`	`39 <0> (?)`

MorrisJobke

I like it 👍

skjnldsv · 2018-06-29T14:59:07Z

@MorrisJobke still some fixes to do :)

Find a way to recompile the icons on accessibility theme update
Fix tests

juliusknorr · 2018-07-03T13:04:32Z

lib/private/Template/IconsCacher.php

+
+		$data = '';
+		foreach ($icons as $icon => $url) {
+			$data .= "--$icon: url('$url?v=1');";


Is there a reason that the v=1 is appended here and not at the css level? I think that could lead to icons not being reloaded if they are changed.

I have no idea, they were always like that, so I didn't give it a lot of thought :)

I mean usually we have it inside of the css file, which is fine, since then it can be bumped once the icon file is changed, and the browsers can detect that they should load the file again.

We should probably remove all of those ?v=1 then

Probably yes.

skjnldsv · 2018-07-04T09:28:41Z

Okay, that's how I proceed:
I added a mixin for our scss which automatically handle the svg colour api

.icon-screen-off {
	@include icon-color('screen-off', 'actions', $color-black, true);
}

Basically you just need to add this to your css and the background-image will be automatically added to a variable. The Mixin works that way:

/**
 * SVG COLOR API
 * 
 * @param string $icon the icon filename
 * @param string $dir the icon folder within /core/img if $core or app name
 * @param string $color the desired color in hexadecimal
 * @param bool [$core] search icon in core
 *
 * @returns string the url to the svg api endpoint
 */
@mixin icon-color($icon, $dir, $color, $core: false)

Then, every time the scss is compiled, the system will detect if an icon is used via this mixin and export it to a new css file called icons-vars.css (cached into the appdata)
The accessibility on dark theme automatically parse the icons-vars file and invert the proper colors from fff->000 and 000->fff (we don't reverse any other colours)

@nextcloud/designers what are your thoughts? Is this a good behavior?
@MorrisJobke @jancborchardt @juliushaertl @rullzer

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

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

juliusknorr · 2018-07-19T06:17:27Z

@rullzer Rebased.

MorrisJobke

Unit tests fail:


1) Test\Template\SCSSCacherTest::testProcessUncachedFileNoAppDataFolder

/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:127
/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:245
/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:142
/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:143

2) Test\Template\SCSSCacherTest::testProcessUncachedFile

/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:167
/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:245
/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:142
/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:179

3) Test\Template\SCSSCacherTest::testProcessCachedFile

/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:203
/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:245
/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:142
/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:210

4) Test\Template\SCSSCacherTest::testProcessCachedFileMemcache

/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:242
/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:245
/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:142
/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:249

5) Test\Template\SCSSCacherTest::testGetCachedSCSS with data set #0 ('core', 'core/css/styles.scss', '/css/core/styles.css', '14.0.0 alpha')
Expectation failed for method name is equal to "linkToRoute" when invoked 1 time(s)
Parameter 1 for invocation OCP\IURLGenerator::linkToRoute('core.Css.getCss', Array (...)): string does not match expected value.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'fileName' => '77d8-8422-styles.css'
+    'fileName' => '77d8-0ab8-styles.css'

/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:365
/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:454

6) Test\Template\SCSSCacherTest::testGetCachedSCSS with data set #1 ('files', 'apps/files/css/styles.scss', '/css/files/styles.css', '1.9.0')
Expectation failed for method name is equal to "linkToRoute" when invoked 1 time(s)
Parameter 1 for invocation OCP\IURLGenerator::linkToRoute('core.Css.getCss', Array (...)): string does not match expected value.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'fileName' => 'd16b-8422-styles.css'
+    'fileName' => 'd16b-0ab8-styles.css'

/drone/src/github.com/nextcloud/server/lib/private/Template/SCSSCacher.php:365
/drone/src/github.com/nextcloud/server/tests/lib/Template/SCSSCacherTest.php:454

rullzer · 2018-07-19T17:31:31Z

core/Controller/SvgController.php

+	 * @param string $folder
+	 * @param string $fileName
+	 * @param string $color
+	 * @return DataDisplayResponse|NotFoundException


You don't return an exception you throw it ;)

rullzer · 2018-07-19T17:33:54Z

core/Controller/SvgController.php

+	 * @param string $color
+	 * @return DataDisplayResponse|NotFoundException
+	 */
+	public function getSvgFromCore(string $folder, string $fileName, string $color = 'ffffff') {


You can also declare return types ;)

rullzer · 2018-07-19T17:34:37Z

core/Controller/SvgController.php

+	 * @param string $app
+	 * @param string $fileName
+	 * @param string $color
+	 * @return DataDisplayResponse|NotFoundException


NotFoundResponse it seems

rullzer · 2018-07-19T17:34:50Z

core/Controller/SvgController.php

+			return $this->getSvg($path, $color, $fileName);
+		}
+
+		$appPath = \OC_App::getAppWebPath($app);


Can't the appmanager do this?

rullzer · 2018-07-19T17:37:32Z

lib/private/Template/IconsCacher.php

@@ -0,0 +1,146 @@
+<?php
+/**


rullzer · 2018-07-19T17:43:54Z

tests/lib/Template/IconsCacherTest.php

@@ -0,0 +1,126 @@
+<?php
+/**


Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Done

Signed-off-by: Roeland Jago Douma <[email protected]>

rullzer

Lets do this. 🎸

skjnldsv · 2018-07-21T14:10:59Z

lib/private/Template/IconsCacher.php

+	protected $urlGenerator;
+
+	/** @var string */
+	private $iconVarRE = '/--(icon-[a-z0-9-]+): url\(["\']([a-z0-9-_\~\/\?\&\=\.]+)[^;]+;/m';


@juliushaertl some of the icons are broken because of your last edit.
We shoudl exclude the ?v= from the matches because otherwise it gets re-added on every compile and we get stuff like that: --icon-confirm-fade-000: url('/svg/core/actions/confirm-fade/000?v=2?v=1?v=1?v=1?v=1');

Was there a specific reason? Or can I remove the ?=& from the regex? :)

No, your right, we can remove those. The others might be needed depending on the base URL of the nextcloud installation.

nickvergessen · 2018-07-30T09:33:03Z

core/css/icons.scss

-	background-image: url('../img/actions/video.svg?v=2');
-	filter: invert(100%) drop-shadow(1px 1px 4px var(--color-box-shadow));
+	@include icon-color('video', 'actions', $color-white, 1, true);
+	filter: drop-shadow(1px 1px 4px var(--color-box-shadow));


Is there a bigger reason the invert(100%) was removed? this breaks the icons on the Talk UI, because now the white icon has a white shadow on a white background while you are waiting for a participant to join the call

Invert is not available in IE11 (see #8702)

@skjnldsv Do you have an idea why the shadow would be white? The drop shadow should not be changed to white, right?

skjnldsv · 2018-07-30T09:44:24Z

The shadow? you mean drop-shadow?
It should not be touched. and is set to black on variables.scss

@nickvergessen are you using the proper new css class? icon-video

MorrisJobke · 2018-10-08T13:21:41Z

This causes high load on Safari: #11179

😢

skjnldsv added enhancement design Design, UI, UX, etc. 3. to review Waiting for reviews labels Jun 25, 2018

skjnldsv added this to the Nextcloud 14 milestone Jun 25, 2018

skjnldsv self-assigned this Jun 25, 2018

skjnldsv requested review from jancborchardt and juliusknorr June 25, 2018 09:20

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

skjnldsv mentioned this pull request Jun 25, 2018

Icon inconsistency #5157

Closed

14 tasks

MorrisJobke reviewed Jun 25, 2018

View reviewed changes

MorrisJobke mentioned this pull request Jun 25, 2018

icon-white class doesn't work on IE11 #8702

Closed

jancborchardt reviewed Jun 25, 2018

View reviewed changes

juliusknorr reviewed Jun 29, 2018

View reviewed changes

skjnldsv force-pushed the inverted-svg-api branch from f888fae to 10ebd3b Compare June 29, 2018 11:15

MorrisJobke approved these changes Jun 29, 2018

View reviewed changes

juliusknorr mentioned this pull request Jul 3, 2018

Slideshow icons are not shown in IE11 nextcloud/gallery#439

Closed

juliusknorr reviewed Jul 3, 2018

View reviewed changes

skjnldsv requested review from jancborchardt, juliusknorr and rullzer July 4, 2018 09:29

skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 4, 2018

skjnldsv and others added 7 commits July 19, 2018 08:16

Autoloader bump

59db525

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Fixed tests

14bc9ff

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Add versionning to icon svg color api function

dccfed8

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Fixed svg generator regex && added missing icons

6194681

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Use proper URL prefix for icons

6ed8c19

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

Fix icon url regex

9159ed3

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

Fix settings icon fetching

bfc6fc8

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

juliusknorr force-pushed the inverted-svg-api branch from 5a25b3d to bfc6fc8 Compare July 19, 2018 06:17

MorrisJobke requested changes Jul 19, 2018

View reviewed changes

rullzer previously requested changes Jul 19, 2018

View reviewed changes

Php code bump

37a4d2b

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Fix unit tests

9d94cc1

Signed-off-by: Roeland Jago Douma <[email protected]>

rullzer approved these changes Jul 19, 2018

View reviewed changes

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 19, 2018

MorrisJobke approved these changes Jul 19, 2018

View reviewed changes

MorrisJobke merged commit 4751f1e into master Jul 19, 2018

MorrisJobke deleted the inverted-svg-api branch July 19, 2018 21:54

nextcloud-bot mentioned this pull request Jul 20, 2018

Svg api doesn't overwrite old image versions suffixes #10309

Closed

1 task

juliusknorr mentioned this pull request Jul 20, 2018

Add icon-black-white mixin to automatically generate icon classes #10311

Merged

skjnldsv commented Jul 21, 2018

View reviewed changes

This was referenced Jul 23, 2018

UI polishing nextcloud/firstrunwizard#71

Merged

Use correct icon for Nextcloud 14 nextcloud/activity#282

Merged

Fix icon file names #10378

Merged

nickvergessen reviewed Jul 30, 2018

View reviewed changes

skjnldsv mentioned this pull request Nov 16, 2018

Remove unused svg api route #12487

Merged

Uh oh!

Conversation

skjnldsv commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Jun 26, 2018

Uh oh!

skjnldsv commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Jun 29, 2018

Uh oh!

skjnldsv commented Jun 29, 2018

Uh oh!

codecov bot commented Jun 29, 2018

Codecov Report

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Jul 4, 2018

Uh oh!

juliusknorr commented Jul 19, 2018

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Jun 25, 2018 •

edited

Loading

skjnldsv commented Jun 29, 2018 •

edited

Loading

juliusknorr Jul 30, 2018 •

edited

Loading

skjnldsv commented Jul 30, 2018 •

edited

Loading