Theming using SCSS variables #3531

juliusknorr · 2017-02-18T09:40:31Z

Requires #3256

Custom themes as well as the theming app will get a lot simpler with #3256. We can just overwrite the existing SCSS variables and can drop most of the string concatenated CSS code inside of the ThemingController.

There will be also much less maintenance work for the theming app in order to keep up with any CSS code changes.

@skjnldsv @nextcloud/theming @nextcloud/designers

Custom themes

With this you can apply a different color scheme to custom themes like this:

class OC_Theme {
	public function getScssVariables() {
		return [
			'color-primary' => '#000000',
			'color-primary-text' => '#ffffff',
                        'color-main-text' => '#ffffff',
                        'color-main-background' => '#333333'
		];
	}
}

ToDo

Theming code cleanup
Fix tests

mention-bot · 2017-02-18T09:40:32Z

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

skjnldsv · 2017-02-18T09:42:52Z

apps/theming/lib/Controller/ThemingController.php

-				'background-image: url(\'./logo?v='.$cacheBusterValue.'\');' .
-				'background-size: contain;' .
-				'}' . "\n" .
+				'#header .logo, ' .


Can't we pass the logo as a variable too? :D

Should be possible. I'll have a look. :)

juliusknorr · 2017-02-18T09:43:32Z

lib/private/Template/SCSSCacher.php

+		$injectedVariables = $this->getInjectedVariables();
+		if($injectedVariables !== '' && $this->config->getAppValue('core', 'scss.variables') !== md5($injectedVariables)) {
+			$this->resetCache();
+			$this->config->setAppValue('core', 'scss.variables', md5($injectedVariables));


I would rather like to use ICache here but since there is no caching enabled as a default, we would be generating the css code on every request. It's a little missuse of the appConfig but since it is loaded into memory on every request, there is no overhead when requesting existing files. We only have one database update per changing scss variables. Feedback for that would be nice.

Isn't cache enable my default no matter we have redis/memcache enabled?
Isn't there a filecaching fallback for this purpose only?

yep, I 've been using the ICache function on my fbsync extension since a year and I remember it was working before I enabled redis iirc :)

At least it was not on my simple dev setup.

https://github.com/nextcloud/server/blob/master/lib/private/Server.php#L393-L406 looks like it was because PHPUNIT was enabled. Not sure if it is ok to regenerate everytime in dev setups, it is so slow then :(

https://github.com/nextcloud/server/blob/master/lib/private/Server.php#L393-L406 looks like it was because PHPUNIT was enabled. Not sure if it is ok to regenerate everytime in dev setups, it is so slow then :(

The define there should only trigger if PHPUnit is actually running (e.g. on test execution)

LukasReschke

I like it 🚀

Some nitpicks in the other comments here, mainly about unit tests and questions :)

LukasReschke · 2017-02-20T12:34:45Z

apps/theming/lib/ThemingDefaults.php

@@ -150,6 +154,29 @@ public function getBackground() {
 		}
 	}

+
+	public function getScssVariables() {


Needs PHPDoc and a small unit test :)

LukasReschke · 2017-02-20T12:35:50Z

lib/private/Template/SCSSCacher.php

 	 */
-	public function __construct(ILogger $logger, IAppData $appData, IURLGenerator $urlGenerator, SystemConfig $systemConfig) {
+	public function __construct(ILogger $logger, IAppData $appData, IURLGenerator $urlGenerator, IConfig $config, \OC_Defaults $defaults) {


I'd prefer adding each of them on a newline to keep the code easier to read without having to scroll :)

LukasReschke · 2017-02-20T12:38:51Z

lib/private/Template/SCSSCacher.php

+	 * @param ISimpleFolder $folder
+	 * @return bool
+	 */
+	private function variablesChanged($fileNameCSS, ISimpleFolder $folder) {


Tests please :)

LukasReschke · 2017-02-20T12:39:16Z

lib/private/Template/SCSSCacher.php

+		try {
+			$variablesFile = \OC::$SERVERROOT . '/core/css/variables.scss';
+			$cachedFile = $folder->getFile($fileNameCSS);
+			if ($cachedFile->getMTime() < filemtime($variablesFile)


I suppose this block is because of updates in dev environments?

Right. Otherwise changing anything in variables.scss will not trigger regenerating the css files.

LukasReschke · 2017-02-20T12:39:52Z

lib/private/Template/SCSSCacher.php

+	 * Reset scss cache by deleting all generated css files
+	 * We need to regenerate all files when variables change
+	 */
+	public function resetCache() {


Does this have to be public?

I made those public so i can add unit test for those easily later on.

You can also test private methods. We have a helper for that, check \Test\TestCase::invokePrivate and it's usages :)

But we can also cover them just with the existing public methods.

Ah nice 👍 I wasn't aware of that.

LukasReschke · 2017-02-20T12:40:11Z

lib/private/Template/SCSSCacher.php

+	/**
+	 * @return string SCSS code for variables from OC_Defaults
+	 */
+	public function getInjectedVariables() {


Does this have to be public?

Fixed

juliusknorr · 2017-03-24T11:11:03Z

Time for review.

juliusknorr · 2017-03-24T11:11:31Z

@nextcloud/theming

nickvergessen · 2017-03-24T11:44:45Z

apps/theming/lib/ThemingDefaults.php

@@ -58,27 +60,30 @@ class ThemingDefaults extends \OC_Defaults {
 	 * @param IL10N $l
 	 * @param IURLGenerator $urlGenerator
 	 * @param \OC_Defaults $defaults
-	 * @param IRootFolder $rootFolder
+	 * @param IRootFolder $appData


class needs changing as well

Fixed 😃

LukasReschke

Logo is disappearing with this. Let me try to find the responsible code :)

GET http://10.211.55.7/stable9//stable9/core/img/logo.svg?v=1 404 (Not Found)

The one /stable9/ seems too much.

codecov-io · 2017-03-26T14:24:29Z

Codecov Report

Merging #3531 into master will decrease coverage by 51.33%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master   #3531       +/-   ##
============================================
- Coverage     53.96%   2.62%   -51.34%     
- Complexity    21679   21694       +15     
============================================
  Files          1272    1272               
  Lines         75789   75096      -693     
============================================
- Hits          40898    1970    -38928     
- Misses        34891   73126    +38235

Impacted Files	Coverage Δ	Complexity Δ
lib/private/legacy/defaults.php	`0% <0%> (-38.24%)`	`44 <2> (+2)`
lib/private/Server.php	`3.75% <0%> (-89.46%)`	`120 <0> (ø)`
lib/private/Template/SCSSCacher.php	`0% <0%> (-76.6%)`	`34 <11> (+12)`
apps/theming/lib/Controller/ThemingController.php	`0% <0%> (-78.2%)`	`27 <0> (-5)`
lib/private/TemplateLayout.php	`0% <0%> (ø)`	`35 <0> (+2)`	⬆️
themes/example/defaults.php	`0% <0%> (ø)`	`17 <1> (+1)`	⬆️
apps/theming/lib/ThemingDefaults.php	`0% <0%> (-86.05%)`	`33 <4> (+4)`
lib/private/Security/CSRF/CsrfTokenManager.php	`0% <0%> (-100%)`	`8% <0%> (ø)`
apps/dav/lib/Connector/Sabre/Server.php	`0% <0%> (-100%)`	`1% <0%> (ø)`
apps/systemtags/lib/Settings/Admin.php	`0% <0%> (-100%)`	`3% <0%> (ø)`
... and 772 more

LukasReschke · 2017-03-26T15:04:05Z

apps/theming/lib/ThemingDefaults.php

+		];
+
+		$variables['image-logo'] = "'../../".$this->getLogo()."'";
+		$variables['image-login-background'] = "'../../".$this->getBackground()."'";


/* override styles for login screen in guest.css */ /* line 42, /media/psf/stable9/apps/theming/css/theming.scss */ #header .logo, #header .logo-icon { background-size: contain; background-image: url(../../..//stable9/core/img/logo.svg?v=1); }

Generated URL is wrong :)

MorrisJobke · 2017-03-26T22:08:12Z

Failing tests (weirdly there is now a /owncloud/ in the path 😕

There were 3 failures:
1) OCA\Theming\Tests\ThemingDefaultsTest::testGetLogoDefault
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://localhost/core/img/logo.svg'
+'http://localhost/owncloud/core/img/logo.svg'
/drone/src/github.com/nextcloud/server/apps/theming/tests/ThemingDefaultsTest.php:443
2) OCA\Theming\Tests\ThemingDefaultsTest::testGetLogoCustom
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://localhost/index.php/apps/theming/logo'
+'http://localhost/owncloud/index.php/apps/theming/logo'
/drone/src/github.com/nextcloud/server/apps/theming/tests/ThemingDefaultsTest.php:458
3) OCA\Theming\Tests\ThemingDefaultsTest::testGetScssVariables
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'theming-cachebuster' => ''0''
-    'image-logo' => ''http://localhost/index.php/apps/theming/logo''
+    'image-logo' => ''http://localhost/owncloud/index.php/apps/theming/logo''
     'image-login-background' => ''/index.php/apps/theming/logi...round''
     'color-primary' => '#000000'
     'color-primary-text' => '#ffffff'
 )
/drone/src/github.com/nextcloud/server/apps/theming/tests/ThemingDefaultsTest.php:490

rullzer · 2017-04-10T19:51:52Z

Needs a rebase

MorrisJobke · 2017-04-21T20:42:45Z

Needs a rebase

Squashed and rebased, but I haven't tested it.

MorrisJobke · 2017-04-21T20:43:42Z

Just in case I messed something up I also have a local backup branch available ;)

juliusknorr · 2017-04-22T09:59:28Z

@MorrisJobke Works as expected. I've just added another commit that ensures that the css files will be reloaded if theming values are changed. Ready for review, I think. @nextcloud/theming

Outdated

nickvergessen · 2017-04-24T10:04:24Z

apps/theming/lib/ThemingDefaults.php

 		}

-		return $this->urlGenerator->linkToRoute('theming.Theming.getLogo') . '?v=' . $cacheBusterCounter;
+		return $this->urlGenerator->linkToRouteAbsolute('theming.Theming.getLogo') . '?v=' . $cacheBusterCounter;


Breaks HTML mails, will have a look.

MorrisJobke · 2017-04-24T14:15:07Z

There was 1 failure:
1) OCA\Theming\Tests\ThemingDefaultsTest::testGetLogoDefault
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'/core/img/logo.svg?v=0'
+'/owncloud/core/img/logo.svg?v=0'
/drone/src/github.com/nextcloud/server/apps/theming/tests/ThemingDefaultsTest.php:457

@nickvergessen Could you have a look?

rullzer · 2017-04-24T19:34:20Z

apps/theming/tests/ThemingDefaultsTest.php

@@ -431,33 +454,61 @@ public function testGetLogoDefault() {
 			->method('getFolder')
 			->with('images')
 			->willThrowException(new \Exception());
-		$expected = $this->urlGenerator->imagePath('core','logo.svg') . '?v=0';
-		$this->assertEquals($expected, $this->template->getLogo());
+		$this->assertEquals('/core/img/logo.svg?v=0', $this->template->getLogo());


Yeah this breaks because https://github.com/nextcloud/server/blob/master/apps/theming/lib/ThemingDefaults.php#L144 adds the \OC::$WEBROOT but https://github.com/nextcloud/server/blob/master/apps/theming/lib/ThemingDefaults.php#L169 does not.

Which feels very wrong...

both should add the webroot...

I adjusted the test

Signed-off-by: Julius Haertl <[email protected]> Add Scss variables to example theme and theming app Signed-off-by: Julius Haertl <[email protected]> Use SCSSCacher to build theming css Signed-off-by: Julius Härtl <[email protected]> Update theming.scss Signed-off-by: Julius Härtl <[email protected]> Code cleanup Signed-off-by: Julius Härtl <[email protected]> Fix tests Signed-off-by: Julius Härtl <[email protected]> Inject SCSSCacher for easier testing Signed-off-by: Julius Härtl <[email protected]> Fix typehint Signed-off-by: Lukas Reschke <[email protected]> Generate absolute URLs Signed-off-by: Lukas Reschke <[email protected]> Fix tests to always use absolute urls for theming images Signed-off-by: Julius Härtl <[email protected]> MailheaderColor -> ColorPrimary Signed-off-by: Roeland Jago Douma <[email protected]>

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

They are used way to often for such a change Signed-off-by: Joas Schilling <[email protected]>

nickvergessen · 2017-04-25T09:46:37Z

apps/theming/tests/ThemingDefaultsTest.php


 	public function setUp() {
 		parent::setUp();
 		$this->config = $this->getMockBuilder(IConfig::class)->getMock();
 		$this->l10n = $this->getMockBuilder(IL10N::class)->getMock();
-		$this->urlGenerator = $this->getMockBuilder(IURLGenerator::class)->getMock();
+		$this->urlGenerator = \OC::$server->query(IURLGenerator::class);


why was this replaced anyway... I will revert this as well

Signed-off-by: Joas Schilling <[email protected]>

jancborchardt · 2017-04-25T12:34:16Z

@juliushaertl @nickvergessen @rullzer @skjnldsv is there anything specifically blocking it, or anything to change/add? Seems to work well. :)

MorrisJobke · 2017-04-25T13:48:21Z

Failing test is due to the JS unit problem - see #4494 for a fix.

MorrisJobke

Tested and works 👍

juliusknorr added 2. developing Work in progress enhancement feature: theming labels Feb 18, 2017

juliusknorr added this to the Nextcloud 12.0 milestone Feb 18, 2017

juliusknorr requested a review from skjnldsv February 18, 2017 09:40

skjnldsv reviewed Feb 18, 2017

View reviewed changes

juliusknorr commented Feb 18, 2017

View reviewed changes

skjnldsv added the feature: scss label Feb 18, 2017

LukasReschke previously requested changes Feb 20, 2017

View reviewed changes

juliusknorr force-pushed the theming-scss branch 5 times, most recently from 4afee67 to 4d0386a Compare March 24, 2017 10:05

juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 24, 2017

juliusknorr requested a review from MorrisJobke March 24, 2017 11:11

juliusknorr mentioned this pull request Mar 24, 2017

Inputs have wrong border color with theming #3211

Closed

nickvergessen previously requested changes Mar 24, 2017

View reviewed changes

LukasReschke previously requested changes Mar 26, 2017

View reviewed changes

LukasReschke reviewed Mar 26, 2017

View reviewed changes

juliusknorr mentioned this pull request Mar 28, 2017

Theming retieving Logo bug #4111

Closed

juliusknorr force-pushed the theming-scss branch from bd61522 to 2b0ac50 Compare March 28, 2017 16:43

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 13, 2017

MorrisJobke force-pushed the theming-scss branch from 1039404 to 72fc360 Compare April 21, 2017 20:42

juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 22, 2017

nickvergessen requested changes Apr 24, 2017

View reviewed changes

nickvergessen force-pushed the theming-scss branch from 4e4cd89 to abbd9fb Compare April 24, 2017 10:24

nickvergessen approved these changes Apr 24, 2017

View reviewed changes

nickvergessen force-pushed the theming-scss branch from abbd9fb to 6bf8fee Compare April 24, 2017 10:26

rullzer reviewed Apr 24, 2017

View reviewed changes

juliusknorr and others added 4 commits April 25, 2017 11:39

Use theming cachebuster for server resources

1c54463

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

Fix theming tests

bc1efdf

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

Logo and background URLs must be relative

bedd500

They are used way to often for such a change Signed-off-by: Joas Schilling <[email protected]>

nickvergessen force-pushed the theming-scss branch from 6bf8fee to bedd500 Compare April 25, 2017 09:43

nickvergessen reviewed Apr 25, 2017

View reviewed changes

Mock it

6448cf5

Signed-off-by: Joas Schilling <[email protected]>

MorrisJobke approved these changes Apr 25, 2017

View reviewed changes

MorrisJobke merged commit 5a9224f into master Apr 25, 2017

MorrisJobke deleted the theming-scss branch April 25, 2017 13:56

tw- mentioned this pull request Oct 4, 2017

NC12 slow with multiple domains #6752

Closed

0403350035 mentioned this pull request Dec 1, 2023

[Snyk] Upgrade @vueuse/components from 10.5.0 to 10.6.0 0403350035/server#2

Open

Theming using SCSS variables #3531

Theming using SCSS variables #3531

Conversation

juliusknorr commented Feb 18, 2017 • edited by MorrisJobke Loading

Custom themes

ToDo

mention-bot commented Feb 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasReschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasReschke Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusknorr commented Mar 24, 2017

juliusknorr commented Mar 24, 2017

Choose a reason for hiding this comment

LukasReschke left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 26, 2017 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

MorrisJobke commented Mar 26, 2017

rullzer commented Apr 10, 2017

MorrisJobke commented Apr 21, 2017

MorrisJobke commented Apr 21, 2017

juliusknorr commented Apr 22, 2017

Choose a reason for hiding this comment

MorrisJobke commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jancborchardt commented Apr 25, 2017

MorrisJobke commented Apr 25, 2017

MorrisJobke left a comment

Choose a reason for hiding this comment

juliusknorr commented Feb 18, 2017 •

edited by MorrisJobke

Loading

LukasReschke Feb 20, 2017 •

edited

Loading

codecov-io commented Mar 26, 2017 •

edited by codecov bot

Loading