Theme selector on the client

[astorije]

This adds one of the 2 features mentioned in #5 (comment), yup.

Beyond the obvious benefits from the user's perspective, this will help contributors/team members:

• Test PRs with different themes: we have a history of breaking our own themes because we didn't test them right. No need to restart the server now, which may be a pain when testing content based on history.
• Move the aesthetic choices (colors, sizes, margins/paddings, etc., everything non-system specific) outside of style.css: at the moment, users who want to set custom themes have to add the @import 'themes/<theme>.css'; hack into their custom CSS. Currently themes spend a lot of effort canceling out design choices set in style.css so that's fine, but as we make themes "independent", this hack will not reliably work anymore.

More information in commit messages.

Results

Example	Crypto	Morning	Zenburn

[williamboman]

Nice! Would it be possible to save the theme selection in the user's config file?

[AlMcKinlay]

I've had a quick look through the code, and I'll test it later, but just a quick point, can we not remove the default theme from the settings, because I have a feeling you'll get a flash on page load if you use a non default theme until Java script loads and chooses the theme. Also, I don't want to have to change theme on every machine. But I'll test it later.

[astorije]

can we not remove the default theme from the settings, because I have a feeling you'll get a flash on page load if you use a non default theme until Java script loads and chooses the theme

I haven't noticed such flash but I'll do some more research/testing. Note that the default config is not the right place to solve this if there is one, and there will be one whether or not I leave this in the defaults.

@williamboman: Nice! Would it be possible to save the theme selection in the user's config file?
@YaManicKill: Also, I don't want to have to change theme on every machine.

Actually, this is on purpose, for several reasons:

• It's not unrealistic to use different themes whether you are on desktop or mobile. Actually, if there is a theme that's specifically targeted for Android and does it well but doesn't care about laptops, I'll take it! :-)
• Custom CSS is not (yet) stored on the server, so it would be very bizarre to store one and not the other. Same with every other client settings, except for password change (because obviously) and custom highlights (because unread mechanisms happen on the server).
• This will be more appropriate as part of Settings overhaul #275 whenever we get to it.

So I'd strongly argue it should stay that way for consistency with the other settings and for usability purposes. Plus it really should not be an administrator decision to pick what background color you have to look at on your IRC client :D
The cost is to pick your theme on all devices you use, which is a very small cost overall.

[AlMcKinlay]

Actually, this is on purpose, for several reasons:

Sorry, that's not what I mean.

I'm not suggesting that we have it sync to the server, that's a different issue (which as you say, should happen in #275)

However, if we have no "default" theme set on the user settings, which we use as the default for the browser, we aren't leaving that up to the user, we are enforcing the default to be the lounge default. Sure, I can change that on every machine that I use, and it will stay that theme once I change it on that machine, but we shouldn't take away the option of defining a default one in the first place.

All I'm asking is that we keep the setting for a user's default theme, set that as the default on the page. After that, everything else stays the same.

[astorije]

However, if we have no "default" theme set on the user settings, which we use as the default for the browser, we aren't leaving that up to the user, we are enforcing the default to be the lounge default. Sure, I can change that on every machine that I use, and it will stay that theme once I change it on that machine, but we shouldn't take away the option of defining a default one in the first place.

I actually have the opposite point of view on this and don't think we should do this.

First of all, with the addition of this PR, there is not much point in doing so. Actually, the only benefit would be not changing anything for the existing users and considering we only ship 4 themes and a major version, I really don't have an issue with that. If themes had been client-side from the beginning, making a default theme on the server-side would have never been an enhancement request we would have accepted :D

Now, on the longer term, picture us in a world where you can easily add themes to The Lounge. We'll want to ship it with at least one theme that you cannot remove. In terms of enhancements, it makes sense to have an official theme that we know works flawlessly with every new version, and other themes are separately maintained (maybe under the same organization, but still).

Anyway, I'm losing myself here. My point is: it's pointless to have the administrator-side make decision on what theme users will use, and it's even more pointless now that users can super easily change them.
On top of everything, that would be confusing to have 2 places to configure the current theme and no other client settings have defaults set on the server.

[AlMcKinlay]

it's pointless to have the administrator-side make decision on what theme users will use

It isn't pointless until we get #275 sorted, and settings can be synced. It's especially not pointless if you thin kthat the majority of people running the lounge are not running it on a hosted version run by someone else, but are running their own version. Then it's not "an administrator", but rather it is themselves.

I agree that it will be pointless once we have settings syncing, but until then we are removing a feature, which I highly disagree with.

In terms of enhancements, it makes sense to have an official theme that we know works flawlessly with every new version

Yeah, but we already have that. If a user doesn't set the default theme for themselves on the server, it is set as a default to be the default that we decide. But if we don't have an option on the server, the user will still have a default, but instead of set by them (or their administrator) it is set by us.

[AlMcKinlay]

By the way, here is a copy of the flash I was talking about. Notice how it is white to start with, because it doesn't use the selected theme until Javascript is fully loaded and we have the user settings. If you have a server-side theme, then this doesn't happen as the theme is loaded from as soon as we get any html, so it always uses the right css. If you choose a different theme on 1 computer, then obviously you'll still get that flash, but there's no way around that, tbh.

This might not annoy you, but it annoys me a ridiculous amount.

[maxpoulin64]

I totally agree with @YaManicKill here. We should still keep the default theme config, and simply replace the loaded stylesheet when the user logs in, even if it wasn't for the flash. There are many reasons the theme should apply before the user logs in. This is also needed to set the default theme for public instances.

[astorije]

the majority of people running the lounge are not running it on a hosted version run by someone else, but are running their own version. Then it's not "an administrator", but rather it is themselves.

Well, that's a different story. This is not what The Lounge is, and making this assumption to build features means you are likely to misrepresent users that are not their own administrators.

I agree that it will be pointless once we have settings syncing, but until then we are removing a feature, which I highly disagree with.

Absolutely not. We are changing a feature, improving it even. And making it more consistent with all other user preferences.
And even when settings can sync, this is a different feature you are talking about, as someone's server-synced theme will only be applied after logging in, while you are defining the theme sent by the server before all loading.

If you have a server-side theme, then this doesn't happen as the theme is loaded from as soon as we get any html, so it always uses the right css.

This shows again that you are seeing The Lounge as a one user/administrator system.
So great, you set the default theme to the theme you want to use yourself so that you don't see a flash. Then all your users see a black to white flash if they use the example theme. Or the opposite.
As a matter of fact, users already see flashes on mobile: from the starting page to the loading page and from the loading page to the "themed" page.
Also, with theme being applied after authorizing (there is a bug unrelated to this PR at the moment, will fix), users see your default theme until they login. Very sloppy.
Also, that same flash is likely to happen if people set custom CSS, for example setting one that... changes the theme!

In a nutshell: I'm all for finding ways to reduce the flash issue, but having a default theme on the server configuration will not solve it. At best, it will hide for yourself in your very own configuration.
It seems to me that all your arguments are more personal preferences based on your usage of the app and not very objective.

Seriously, theme on the admin-side of things is not the right way to go. I don't see you complain about admin-side defaults for joins/quits, for custom CSS, for colored nicknames. All these have app defaults, pattern which this PR is applying for theme as well and makes a user decision consistent. Yes it's a change, but not one to be against really. In practice, setting it to your N devices will take you a couple minutes at worst, and then you're done. Forever maybe.

Honestly, I don't understand there is that much debate around a detail like that. I don't see it objectively evolve as your opinion is set, so I won't debate any further on this very point and focus on all the other things there is to do. If you're very much against this PR, please close it and do something better.

Open to feedback otherwise.

[astorije]

We should still keep the default theme config

... Because... ?

There are many reasons the theme should apply before the user logs in.

... Such as... ?

This is also needed to set the default theme for public instances.

Needed? No.
That the administrator wants to enforce a specific style, maybe, but that's a different story.

Again, I don't see why we are arguing in favor of inconsistency.

[astorije]

This is a somewhat revert of #474, which was fixed at the time but tested on an outdated master, so it was not needed anymore.

[astorije]

I re-added the default theme partly because @maxpoulin64 finally gave the first compelling argument on the subject (but also because I'm done fighting): if you own a bunch of services and you want to provide to your users The Lounge with a similar set of colors to others.
Very niche if you ask me but whatever, why not.

Note that I haven't tested changes around custom default theme, so feel free to test your use cases.

[vectr0n]

Does The Lounge need to be restarted for new themes to be picked up for the theme selector?

[astorije]

@vectr0n, with the theme selector on the client added by this PR, it doesn't need server restart (it does need restart on master currently). It doesn't even need reload, it changes when selecting it :-)

---

EDIT:

01:14 vectr0n|cloud what i ment was, if you put a new theme in the theme dir, do you have to restart thelounge to pickup that new theme as an option for users?
01:14 +astorije Aaah my bad, yes, sorry
01:14 vectr0n|cloud no worries, ty
01:14 +astorije There is currently no alternatives for that

[AlMcKinlay]

This shows again that you are seeing The Lounge as a one user/administrator system.
So great, you set the default theme to the theme you want to use yourself so that you don't see a flash. Then all your users see a black to white flash if they use the example theme. Or the opposite.

Huh, I had never noticed this was a global default, rather than per-user. I would be happy with it moving to the user config.

I don't see you complain about admin-side defaults for joins/quits, for custom CSS, for colored nicknames.

No, because they are fundamentally different. When I set those settings on my client, they never change back. But with the theme, the default theme is set for milliseconds every single boot. The other ones having a different default only affects me when I go onto a new machine.

I am in favour of "inconsistency" because the theme works differently to everything else.

It drives me insane seeing that flash every single time I load the page.

Also, with theme being applied after authorizing (there is a bug unrelated to this PR at the moment, will fix), users see your default theme until they login. Very sloppy.

Except that if we don't have this, then they will see ... the default theme until they login. The other solution is no different in this respect.

As a matter of fact, users already see flashes on mobile: from the starting page to the loading page and from the loading page to the "themed" page.

There is 1 flash from pressing the button to having the page actually load something, I accept that. However, it doesn't happen over refreshes (the default theme thing does) and there's (as far as I know) nothing we can do about that. Not ideal, but not as bad as the issue here.

I didn't want to continue arguing after you've fixed it, but I think you are being incredibly unfair with how you are putting my points across.

And the thing is, you are under the false presumption that us not having a default theme in the config means there is no default theme. This is fundamentally false. It just means that instead of the administrator deciding the default theme, we decide it, and they can't override it. The default is still there.

[maxpoulin64]

Looks good to me! The only think I notice is that the box grey, which makes it looks like it's disabled. Maybe theme it the same as the other controls?

[astorije]

@maxpoulin64, very good point, I updated my PR accordingly. Also updated PR description to add screenshots for all themes, and rebased with master.

(Wow, AppVeyor finishing before Travis CI, in what world are we living...)

[maxpoulin64]

👍

[AlMcKinlay]

Yeah, looks good. Great to get this feature finally in. 👍 and merging.

Oops, another feature in 2.0.0 without releasing :-P