Skip to content
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

Allow Custom Login Background #11

Closed
gerit1a opened this issue May 23, 2017 · 42 comments
Closed

Allow Custom Login Background #11

gerit1a opened this issue May 23, 2017 · 42 comments

Comments

@gerit1a
Copy link

gerit1a commented May 23, 2017

Allow using a custom login background that can be selected through the theming app before applying the theme.

Could we replace this in core/css/core/_server.scss:
background-image: none !important;

With this:
background-image: url(/index.php/apps/theming/loginbackground) !important;

@gerit1a
Copy link
Author

gerit1a commented May 23, 2017

I originally submitted a pull request, but I wasn't sure if I should or how I should do so? If the pull request is okay, maybe you can reopen and commit?

@mwalbeck
Copy link
Owner

there were no problems with your pull request so no worries there. I do have one question, how would one change the background while using a custom theme? because I can't seem to recall/find a way to interact with the theming app while using a custom theme.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

There is no true way, per say. You can just disable the theme in the server config, access the theming app, then re enable the theme. You can also manually edit the background and logo in the nextcloud data/appdata/theming/images.

Also after looking into the theming app's code, it looks like there may be a way to disable the check on a custom theme. This would allow us to use the app to change the background/logo even with a custom theme. I'll look into it.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

Okay in apps/theming/lib/Settings/Admin.php on line 62 change it from this:
if ($theme !== '') { $themable = false; $errorMessage = $this->l->t('You are already using a custom theme'); }
to
/*if ($theme !== '') { $themable = false; $errorMessage = $this->l->t('You are already using a custom theme'); }*/

This might make the file integrity check fail though...
EDIT:
Looks like doing this will allow the theme app to function, however changing anything with it will remove the theme from the config.

EDIT 2:
Apparently it doesn't remove the theme from the config. It just refreshes it without the theme enabled. Refreshing the page gets the theme back. I think its this line of code at apps/theming/js/settings-admin.js line 65. Haven't figured out how to change it just yet though.

@mwalbeck
Copy link
Owner

I do like the idea, but other than the fact that changing the background is a little wonky, I've noticed that the theming app is creating some inconsistencies with icons for custom themes. More specifically the icons for folders and files in the files app.

I'm going to start recommending people disable the theming app as long as there are inconsistencies in the icons used, and look into why that is. As well as looking into a way to change the background without disabling the theme first.

These changes should preferably be done upstream.

Nonetheless, I do intend to merge your pull request after I get a better understanding for what is required for the above. It should be merged before the NC 12 code is moved to master.

If you find anything interesting regarding the above please let me know.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

What inconsistencies are there with the folders? Are they not supposed to be blue?

@mwalbeck
Copy link
Owner

Yes, but sometimes they show up as purple instead which is the folder icon from the example theme that comes with NC. It is also noted in the NC docs that you should disable the theming app when using a custom theme.

@mwalbeck
Copy link
Owner

How about this, we change the background-image url to

url("../../background") !important;

then to change the background you place a file named background (no extension) in the root of the nextcloud-breeze-dark folder. That way we don't need the theming app at all for this to work.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

I'm unable to reproduce the purple folders. Can you tell me what you did to get that? I see that the theming app should be disabled but it seems to work fine except for this purple folder problem?

The only disadvantage to that solution is that the background would get overwritten with every update, and it is not as simple as just changing it through the web app.

BTW Is there a way to autoupdate the theme? Through git I'm supposing?

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

Also is there a reason for the double .. /?

Should is just be
url("../background") !important;

I'm not terribly experienced in this field but it's just from the code I have seen.

@mwalbeck
Copy link
Owner

mwalbeck commented May 24, 2017

It is linked to which icons are cached in the data/appdata_* folder. Either the default NC icons or the example theme icons are cached there, and when using the theming app is utilizes whichever icons are cached. If you disable it the icons are taken straight from the theme you are using. I don't know the specific details as to which icons are cached.

If you use git to pull the latest updates then it's not an issue, git will only replace the background file if one is included in the git repo itself.

Yes, the reason for the double "../" is because the css file is in "/core/css" and the image is located in "/" so the we need to move two directories up to get to the image file.

the background-image rule should be

background-image: url("../../background") !important;

and if you could also add background to the .gitignore file, on a new line, in your pull request then git wont care about the file at all and will just leave it be.

@mwalbeck
Copy link
Owner

screenshot_20170524_182113

and here is an example of the purple folders when the theming app is enabled

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

It appears that the folders changing colors is a result of changing the "color" option in the theming app. If you click it and select reset to default or 0082C9, the folders turn back to blue. I would actually consider the ability to change folder colors an advantage?

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

So it appears that the icons are actually generated from the color selector in the theming app. Changing the color value will cause it to refresh all of these icons with the new specified color.

It is also probably possible to disable this functionality in the theming app, however like you said, it would preferably be better upstream. I don't see that happening, however, since this functionality is specific to this theme.

@mwalbeck
Copy link
Owner

mwalbeck commented May 24, 2017

Interesting, you are partially correct. You can change the colours of the folders but the change to purple has happen on multiple fresh installs. Every time a new NC version comes out I download the zip from their website and use that for development, and it happens more or less every time. Also having the theming app enabled also means that some of the icons for the different file types are taken from the default theme instead of the custom theme.

I agree that the ability to change folder colours when using a custom theme is good, just not the way you can do it now. I know there are some interesting changes coming in NC 13 which might make the implementation of these things easier, some it's something I plan to look at in the future.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

I would agree that it is less than ideal to have the default icons overwriting the themed icons. Is it the theming app specifically that does this overwriting or how exactly is this done?

Its also very interesting that we are having this issue as well. I'm not sure what happened on my end, but about 20 minutes ago, clicking reset to default would change colors back to 0082C9, now it doesn't. I instead get the purple you have been mentioning: 745BCA. This code here (apps/theming/js/settings-admin.js) is still the same which is odd

	var themingDefaults = {
		name: 'Nextcloud',
		slogan: t('lib', 'a safe home for all your data'),
		url: 'https://nextcloud.com',
		color: '#**0082c9**',
		logoMime: '',
		backgroundMime: ''
	};

When you install NC from scratch is the default folder color purple without any theme enabled? I thought it was always blue.

@mwalbeck
Copy link
Owner

Well that is weird, but if you disable the theming app it should change back to blue.

Correct the folders are blue until i change the theme to the breeze dark theme and then the folders change to purple.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

Oh very interesting. It must have been disabling and re enabling the theme that caused the purple folders on my end. Something is causing the theming app to read purple as the default color once the theme is enabled.

@mwalbeck
Copy link
Owner

Indeed, I think it has something to do with the example theme in the themes folder since it used purple folders as the default, but don't know why. One thing I do know is there is being worked on a fix for this upstream but probably wont be released before NC 13.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

I think it may be related to this somehow

	public function getColorPrimary() {
		return $this->config->getAppValue('theming', 'color', $this->color);

Do you have the issue tracker link for this in NC 13? NC 13 is also a ways away since NC 12 was just released.

@mwalbeck
Copy link
Owner

nextcloud/server#5036

Correct which is one of the reason why I currently want to avoid the theming app.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

I guess your original solution works for now, however, it is not ideal. It would be great if they set a standard for custom themes to allow the theming app to manipulate apps in the web interface.

The only real downside of the original solution though was the folders turning purple. It could be fixed by setting it back to blue in the theming settings.

@mwalbeck
Copy link
Owner

I agree, it's not ideal, but from my point of view it's preferable to avoid the theming app to ensure a consistent experience when using the theme. Now I intend to revisit this at a later date, either when there is better custom theme API or I might consider a 3rd party app for adding this functionality to the web interface if necessary.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

Aha! I think I found the culprit causing the purple folders we were seeing. In defaults.php at the root of the theme,

	public function getMailHeaderColor() {
		return '#745bca';
	}

Specifically the #745bca.

@gerit1a
Copy link
Author

gerit1a commented May 24, 2017

Changing this to

public function getMailHeaderColor() {
		return '#0082C9';
	}

completely fixes it. Its almost like there is some sort of API built in?

I think a few mods to the default.php and theming app would allow the theming app to function properly with this theme.

Specifically, there is this.

I'm also assuming that there is some way to grab settings from the theming app configurations so we could do this

	public function getMailHeaderColor() {
		return FunctiontoGrabColor();
	}

The theming app includes functions like this

	public function getColorPrimary() {
		return $this->config->getAppValue('theming', 'color', $this->color);
	}

which seem very promising. But just taking the code and pasting it in doesn't seem to work...

@gerit1a
Copy link
Author

gerit1a commented May 25, 2017

Adding this
namespace OCA\Theming;
to the top of the code will allow the theme to pull all of the settings from the theming app config. However, it seems to ignore everything after that, so I am not sure how to override some of the information with the themed data.

@mwalbeck
Copy link
Owner

Interesting, didn't expect the getMailHeaderColor function to cause problems with folder colours. It also seems removing it works as well.

The more specific docs on custom themes can be seen here

There is still the issue when the theming app is enabled, the icons for files aren't taken from the custom theme but from the core theme instead.

Also the more I think about it, the less inclined I am to even consider using the theming app, since it was clearly designed not to be used with custom themes.

@gerit1a
Copy link
Author

gerit1a commented May 25, 2017

I've always had the theming app enabled but it was just showing the "you are using a custom theme message" before. The icons seem fine to me? At the very least the favorite icon and pdf icons show? What icons don't work?

@mwalbeck
Copy link
Owner

The icons are working fine per se, but the icons are taken from the wrong place. This means if I make modification to the icons those changes won't show up when using the theme.

@gerit1a
Copy link
Author

gerit1a commented May 26, 2017

Hmm ok. I think only the icons that are not available in the theme are from the theming app?
image

@mwalbeck
Copy link
Owner

I'll try an give a little example. If I change the red PDF icon to be yellow and place that in this theme, you won't see that change if you have the theming app enabled, you will see the old red icon instead. Now if you disable the theming app you will see the new yellow PDF icon.

Now I haven't been very clear on this point, but the icons that are an issue here is only the icons for your own files in NC. So if you upload a PDF, it's the icon that PDF gets that is the issue. If you take a look at the source on lets say the PDF icon you can see it is taken from the core NC images through the theming app. When you disabled the theming app you can see the PDF icon being taken straight from the custom theme.

Now the above with the icons in the settings app is not related to this and I will comment in the other issue.

@gerit1a
Copy link
Author

gerit1a commented May 27, 2017

Right. I see exactly what you mean now. It would be great if they could add some option or support for custom theming in the app upstream.

I'm also sure it would be possible to just modify the app to disable the pulling of certain icons, but it could be messy

There is really no good way about this right?

@mwalbeck
Copy link
Owner

Yes that would be great, but from what I've seen I don't thinks it's likely.

Currently, no, there is no good way to go about it. I've have been thinking of creating a Nextcloud app for custom themes so you could update the theme from within Nextcloud. That would also provide a good base for implementing features like this.

@gerit1a
Copy link
Author

gerit1a commented May 31, 2017

Right, a custom app would be awesome, but it does seem like a lot of work. With the exception of the icons issue, the theming app seems to work pretty well.

Maybe we could just fork it and change it to work nicely with this theme? Just disable the code that deals with the icons and it should be good? Or is it more complicated than that?

@mwalbeck
Copy link
Owner

I'm not ruling out using the theming app as a base, or at least parts of it, but I'll have to look more into what features I want in the long run before I make a decision.

@ToneLune
Copy link

I can not seem to get a background for the login page ...
I modified core/css/core/_server.scss like mwalbeck said.
Should the file (without extension) in PNG, JPG or other format?

@mwalbeck
Copy link
Owner

Have you also compiled the changes from sass to css? That might be the reason it doesn't work. I had a quick look at it again and made a small change to it. Since you sound interested in being able to change the login background I'll commit the changes.

I'll make it so if you have a file called loginbackground.png or loginbackground.jpg in the root of the theme folder it should display that image as the background. I'll also, as before, add the two files to the gitignore file.

@mwalbeck
Copy link
Owner

I've added the required changes to the newest release so I'll close this for now. I'll most likely look at it again if a better option becomes available from upstream Nextcloud.

@ToneLune
Copy link

Perfect, it works now !
Thanks to you, I discovered and understood what SASS is.
Good work ! :)

@mwalbeck
Copy link
Owner

Hi,

Just a little update regarding this issue. I've noticed that you can actually use the theming app in Nextcloud 14 with a custom theme. I haven't look to much into it, but I've tested changing the login background, which works. Is enabling this, something that would be of interest?

@ToneLune
Copy link

Hello,

It's a very good news!
I have not tried it since I have not updated to 14.x
I will test next week.

Thanks for continuing to developed this great theme! ;)

@ToneLune
Copy link

Theming app works great with you theme now, congratulations !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants