CSS classes in themes for nick colors #325

astorije · 2016-05-12T06:48:31Z

Advantages compared to the current system:

Individual colors can be tweaked color by color in the theme if one appears to be problematic (too light, too dark, too close to background color, ...)
Each theme can control their colors entirely, not relying on the "system" ones. One can create a monochrome theme, or an accessible theme (with only very contrasted colors)
Themes can define a subset of colors by assigning multiple classes to a same color or filtering using CSS selectors
Custom CSS can override a class, for example if a user doesn't like their own color
Less inline style
One less library

Main drawback of course is new themes have to ship between a few colors up to 64 (but that's the whole point 😄). A quick HTML tool to generate classes will be very easy to do in a few lines of code using randomColor package or whatever solution so that theme authors can bootstrap this in no time.

About the colors themselves:

Colors for the default style (and therefore for crypto as well) have been generated based on the existing code. It's difficult to tell the difference between master and this PR. However, we have the same issues than current system, meaning some are too bright, etc.
Colors for Zenburn and Morning have been built using the randomColor package featured in Generate better nick colors #322
I set up the 64-color threshold arbitrarily. We can do more, we can do less, but note that whatever the generator (current code or the randomColor package), I noted a lot of "visual semi-duplicates". My guess is 32 should be enough, but it doesn't really matter.

Screenshots:

Example before	Example after	Morning before	Morning after

In situ:

Example	Morning

astorije · 2016-05-12T06:52:35Z

client/js/libs/handlebars/colorClass.js

+
+		return "color-" + hash % 64;
+	}
+);


This can definitely be improved with all sorts of crazy hashing. However, considering the end goal, I don't think having a not-perfectly-random solution is not a big deal.

Also, I didn't cache classes as we currently do. I don't think this is going to affect performance though. A fast hashing function (fast over randomness quality) is probably the way to go anyway.

Suggestions welcome :-)

AlMcKinlay · 2016-05-12T07:35:56Z

This looks pretty good to me. The only issue is that the dark theme colours are quite similar (it looks like we have maybe 6 or 7 distinct colours). However, I think getting this PR in first and improving the colours over time would be a good plan, because currently it's very hard to see nearly half of the usernames on the dark themes.

xPaw · 2016-05-12T13:12:25Z

client/css/style.css

@@ -752,6 +752,71 @@ button,
 	color: #50a656 !important;
 }

+#chat .user.color-1 { color: #1396cf; }


Your indexes are off. Either you need to add 1 in colorClass function or re-index classes to be 0-63.

Oops my bad, good catch. Fixed in the colorClass function.

astorije · 2016-05-12T15:31:34Z

Just out of curiosity, I tried reducing from 64 colors to 32.

Example 64	Example 32	Morning 64	Morning 32

Seeing this, I think 32 colors are enough. Shuffle them and you won't be able to tell which is which :-)
(Also, don't pay attention to your own color, if hashing function changes before merging, the index does as well, plus we'll probably change colors before merging)

@thelounge/maintainers, what do you think? 32 or 64? I'm fine either way, leaning towards 32 though.

AlMcKinlay · 2016-05-12T15:55:14Z

I think 32 is probably fine. The colours look much better and less similar, and very rarely are you going to have 32 people chatting for it to be a problem. My vote for 32.

xPaw · 2016-05-12T18:29:48Z

32 is OK. I looked at HexChat, and they have mere 9.

@astorije Change no-color to color (inverted option) as we discussed, and then this can be merged.

These colors were built using the current generation function to have similar style.

These colors have been generated by the randomColor package

This will reset users' preference regarding colored nicknames but it's to make it more specific than just "colors".

astorije · 2016-05-15T20:02:23Z

Alright, I'm all set @xPaw! Let's ship it, I have further unrelated improvements to make but I don't want to do that before it gets merged :-)

xPaw · 2016-05-15T20:03:34Z

client/js/lounge.js

@@ -437,7 +437,7 @@ $(function() {
 	var settings = $("#settings");
 	var options = $.extend({
 		desktopNotifications: false,
-		colors: false,
+		coloredNicks: true,


Don't rename the option, as you break previously stored setting.

Just for the record:

16:07 +xPaw astorije, looks OK except for the renamed settings, i'll test it today or tomorrow
16:08 +astorije xPaw, what's wrong with the renamed settings§?
16:08 +xPaw backwards compat
16:08 +xPaw I would rather leave renaming settings to Max-P's PR
16:08 +astorije I know it breaks settings, but if we are jumping 2.0.0 I'm not too complexed about it
16:09 +astorije To be honest, I am not too happy having a class that is system specific like that simply called "colors"
16:09 +astorije And leaving the renaming to Max-P's PR means changing 32 * 4 lines
16:09 +xPaw i specifically mean the setting name
16:09 +xPaw class can stay
16:09 +astorije Oh right, my bad
16:09 +astorije Yeah
16:10 +astorije I'd say the earlier the better, especially if we jump 2.x
16:10 +astorije If we end up not doing it (which I doubt more and more), then fine, but otherwise...
16:10 +xPaw eh, fuck it, fine
16:10 +xPaw i'll test it later to make sure nothing broke
16:10 +astorije lol
16:10 +astorije Sure
16:10 +astorije I tested the 4 themes in private browsing fyi

xPaw · 2016-05-16T08:12:53Z

👍 Looks all fine to me.

The only unrelated thing I noticed is that we use button in user list, and a in chat. Perhaps we should change all the clickable nicknames to be button? Not in this PR obviously.

That would do two things:

Remove hover with url that browsers display.
Link color that comes from #chat a.

AlMcKinlay · 2016-05-16T08:20:00Z

Perhaps we should change all the clickable nicknames to be button?

Can't do that, it breaks highlighting on firefox. I thought we changed the userlist to use links, but apparently not.

xPaw · 2016-05-16T08:22:02Z

Can't do that, it breaks highlighting on firefox.

Hm, what about <span role="button">?

AlMcKinlay · 2016-05-16T08:28:14Z

Hm, what about ?

I'll have to test that later. I don't run firefox at work anymore (used to, that's how I found the bugs). Will check sometime.

astorije · 2016-05-16T20:19:27Z

What about the original PR, @YaManicKill?
I agree with both of you, it should not be a link, but I made it a dedicated issue to discuss testing and consequences, including in accessibility, see #338.
I'm obviously not planning to add this to the current PR, let's ship it! :)

AlMcKinlay · 2016-05-17T07:15:22Z

What about the original PR, @YaManicKill?

This PR? Yeah, I'm happy with it. 👍

I'll merge as this is second review. Good job, @astorije I'm really looking forward to having this in release.

CSS classes in themes for nick colors

astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label May 12, 2016

astorije reviewed May 12, 2016
View reviewed changes

astorije force-pushed the astorije/color-classes branch from e482166 to 7f551f1 Compare May 12, 2016 06:55

xPaw mentioned this pull request May 12, 2016

Generate better nick colors #322

Closed

astorije mentioned this pull request May 12, 2016

Next release planning #326

Closed

xPaw reviewed May 12, 2016
View reviewed changes

astorije force-pushed the astorije/color-classes branch from 7f551f1 to a56e2f5 Compare May 12, 2016 13:56

astorije force-pushed the astorije/color-classes branch from a56e2f5 to 826903e Compare May 12, 2016 15:38

astorije force-pushed the astorije/color-classes branch 3 times, most recently from c2f63c5 to 62a8417 Compare May 13, 2016 05:05

xPaw added this to the ★ Next Release milestone May 13, 2016

astorije added 3 commits May 15, 2016 15:21

Replace color generation with CSS classes

c87d82d

These colors were built using the current generation function to have similar style.

Add light colors for dark themes Morning and Zenburn

b3103e8

These colors have been generated by the randomColor package

Invert no-colors CSS class and rename option

1af00d3

This will reset users' preference regarding colored nicknames but it's to make it more specific than just "colors".

astorije force-pushed the astorije/color-classes branch from 62a8417 to 1af00d3 Compare May 15, 2016 19:58

xPaw reviewed May 15, 2016
View reviewed changes

xPaw added the second review needed label May 16, 2016

astorije mentioned this pull request May 16, 2016

Change nicks from links to buttons everywhere #338

Closed

AlMcKinlay merged commit 304314d into master May 17, 2016

AlMcKinlay removed the second review needed label May 17, 2016

AlMcKinlay deleted the astorije/color-classes branch May 17, 2016 07:15

astorije assigned AlMcKinlay May 21, 2016

AlMcKinlay mentioned this pull request May 27, 2016

Non-colored nicknames should be disabled and colors should be theme-specific #45

Closed

astorije mentioned this pull request May 31, 2016

Remove design-specific nickname colors from base system style #368

Closed

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#325 from thelounge/astorije/color-classes

3377c40

CSS classes in themes for nick colors

AlMcKinlay removed their assignment Mar 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSS classes in themes for nick colors #325

CSS classes in themes for nick colors #325

astorije commented May 12, 2016

astorije May 12, 2016 •

edited

AlMcKinlay commented May 12, 2016

xPaw May 12, 2016

astorije May 12, 2016 •

edited

astorije commented May 12, 2016

AlMcKinlay commented May 12, 2016

xPaw commented May 12, 2016

astorije commented May 15, 2016

xPaw May 15, 2016

astorije May 15, 2016

xPaw commented May 16, 2016 •

edited

AlMcKinlay commented May 16, 2016

xPaw commented May 16, 2016 •

edited

AlMcKinlay commented May 16, 2016

astorije commented May 16, 2016 •

edited

AlMcKinlay commented May 17, 2016

CSS classes in themes for nick colors #325

CSS classes in themes for nick colors #325

Conversation

astorije commented May 12, 2016

astorije May 12, 2016 • edited

Choose a reason for hiding this comment

AlMcKinlay commented May 12, 2016

xPaw May 12, 2016

Choose a reason for hiding this comment

astorije May 12, 2016 • edited

Choose a reason for hiding this comment

astorije commented May 12, 2016

AlMcKinlay commented May 12, 2016

xPaw commented May 12, 2016

astorije commented May 15, 2016

xPaw May 15, 2016

Choose a reason for hiding this comment

astorije May 15, 2016

Choose a reason for hiding this comment

xPaw commented May 16, 2016 • edited

AlMcKinlay commented May 16, 2016

xPaw commented May 16, 2016 • edited

AlMcKinlay commented May 16, 2016

astorije commented May 16, 2016 • edited

AlMcKinlay commented May 17, 2016

astorije May 12, 2016 •

edited

astorije May 12, 2016 •

edited

xPaw commented May 16, 2016 •

edited

xPaw commented May 16, 2016 •

edited

astorije commented May 16, 2016 •

edited