Add autocomplete strategy for foreground and background colors #1109

astorije · 2017-04-28T21:46:10Z

Addresses #1105

One limitation of this is it only works for foreground color. Making it support background color as well is not impossible, but it's more efforts and I can't spend them on that right now. I still think the current PR adds some value anyway! Well, not anymore it doesn't.

Color labels were taken from https://modern.ircdocs.horse/formatting.html#colors, which we link to from the client.

xPaw · 2017-04-29T06:15:36Z

client/js/lounge.js

+			callback(matchingColorCodes);
+		},
+		template(value) {
+			if (value === colorCodeMap[0]) {


This is kinda theme dependant, probably better to handle in css.

Agreed, but I couldn't find a good way to do that simply without having to duplicate the entire .irc-(f|b)g* CSS palette, which I really don't want to do.
An alternative is to... not care, and first line is going to be white text on white bg + non-white hover.
Any suggestions?

.textautocomplete-item .irc-fg1 { background: #000; } something like that.

Duh, of course 😊
But I actually went for white on default background, for several reasons: white on black looks gross in terms of UI, I reuse the text color for the background color dropdown text, the hovering background shows text alright.

Only thing is hovering background is veeeery dim, we really need to change it someday :D

xPaw · 2017-04-29T06:16:02Z

What about autocompleting background colors after you type a comma?

astorije · 2017-04-29T11:25:34Z

What about autocompleting background colors after you type a comma?

I really didn't want to spend one more minute on that to go back to my original tasks, but I guess it's my fault for starting this. Let me try this and if it's more than 5 minutes, I'll give up.

astorije · 2017-04-30T10:44:52Z

Alright alright, I added strategy for background colors. Please don't make me work on this again :D

astorije · 2017-04-30T10:45:20Z

Oh also, updated the screenshot in PR description to show background colors dropdown as well.

xPaw · 2017-04-30T10:47:05Z

client/js/lounge.js

+
+	const foregroundColorStrategy = {
+		id: "foreground-colors",
+		match: /\B\x03(\d{0,2}|[A-Za-z ]{0,10})$/,


Remove \B. You should be able to autocomplete colors no matter where it is.

I didn't work without \B. Feel free to play with it, but I'll leave it as is, I've spent enough time on these regexes I hate 😅.

Ok, it looks like it's working now. I probably messed something with my regexes earlier, nevermind...

xPaw · 2017-04-30T10:48:06Z

client/js/lounge.js

+			return `<span class="irc-fg${value[2]} irc-bg${value[0]}">${value[1]}</span>`;
+		},
+		replace(value) {
+			return "\x03$1," + value[0];


You should add zero padding for <10 colors, so that 1 turns into 01.

I sure can, but do we need to, knowing that it works fine as is? Asking because I don't know the IRC details, you probably know a reason I don't.

https://modern.ircdocs.horse/formatting.html#mistaken-eating-of-text

xPaw · 2017-04-30T10:48:30Z

client/js/lounge.js

+			callback(matchingColorCodes);
+		},
+		template(value) {
+			return `<span class="irc-fg${value[2]} irc-bg${value[0]}">${value[1]}</span>`;


I think the color number should be displayed too. Perhaps the same way emojis are displayed.

I don't think so, it doesn't add value to the autocomplete. If you know the code, you won't rely on the dropdown. It mainly is for user-friendliness.

I think it would help to autocomplete colors faster, as you would see the color number and can type it in, without guessing.

xPaw · 2017-04-30T10:48:51Z

client/js/lounge.js

+			return `<span class="irc-fg${value[0]}">${value[1]}</span>`;
+		},
+		replace(value) {
+			return "\x03" + value[0];


Same here, zero padding.

xPaw · 2017-04-30T10:49:07Z

client/js/lounge.js

+
+	const backgroundColorStrategy = {
+		id: "background-colors",
+		match: /\B\x03(\d{1,2}),(\d{0,2}|[A-Za-z ]{0,10})$/,


Same here, remove \B.

xPaw · 2017-04-30T10:49:37Z

client/js/lounge.js

@@ -111,6 +111,64 @@ $(function() {
 		index: 1
 	};

+	const colorCodeMap = [


Move to constants.js

astorije · 2017-04-30T10:50:56Z

I hesitated with this:

It reflects the reality less but it does look nicer. Thoughts?

xPaw · 2017-04-30T10:51:40Z

@astorije It would look nicer if the entire item/line was colored.

What if you included foreground color name? Like Green on Black.

astorije · 2017-04-30T11:35:57Z

What if you included foreground color name? Like Green on Black.

It looks like this:

(It says "Blue" simply because I hardcoded it for the screenshot)

I'm not sure. What do you think?

astorije · 2017-04-30T11:48:40Z

Alright, really hoping I'm done with this 🙏
I have to move on with life now 😅

xPaw · 2017-04-30T11:54:44Z

client/js/constants.js

@@ -1,5 +1,24 @@
 "use strict";

+const colorCodeMap = [
+	[0, "White"],


I just realized you don't need numbers here. This could simply be an array of color names.

I kinda do for this.
And actually, I'm going to change the first value from a regular int to "01", etc., at least for autocomplete of 0* purposes.

.filter gives you index and value in array, so it should work just fine.

Ok did that, it's much cleaner like this and puts the mapping of color codes/names as a source of truth in the constants.
I could also reconstruct the codes from index + leading zero + toString but it feels hacky. Better to keep the codes here, and also makes it possible to support future stuff like "99".

Please tell me I'm done lol, I so should have never done this in the first place 😅 (anyway, I'm out so I won't be able to work on this more before a day or 2 so that's my cue)

.filter gives you index and value in array, so it should work just fine.

Current (new) solution is cleaner IMO.

xPaw · 2017-04-30T12:15:55Z

@astorije Well, now to get a suggestion for <10 colors, you have type like 05 as typing just 5 will not show a suggestion.

astorije · 2017-04-30T12:17:15Z

@astorije Well, now to get a suggestion for <10 colors, you have type like 05 as typing just 5 will not show a suggestion.

Yes, that will be addressed with fuzzy matching on completion (see #1086).

Please no more bikeshedding. Close this PR if you don't like it but I can't spend another day on this 😅

xPaw · 2017-04-30T12:18:38Z

Don't worry, I'll merge this PR. Also fuzzy matching for colors is pretty lol.

xPaw · 2017-04-30T12:18:51Z

client/js/lounge.js

+			return `<span class="irc-fg${value[2]} irc-bg irc-bg${parseInt(value[0], 10)}">${value[1]}</span>`;
+		},
+		replace(value) {
+			return "\x03$1," + ("00" + value[0]).slice(-2);


Forgot to remove padding.

xPaw · 2017-04-30T12:19:04Z

client/js/lounge.js

+			callback(matchingColorCodes);
+		},
+		template(value) {
+			return `<span class="irc-fg${value[2]} irc-bg irc-bg${parseInt(value[0], 10)}">${value[1]}</span>`;


Do you need to parseInt on value[2]?

Done. But don't you think we should be using irc-fg04 and the likes instead of irc-fg4, to better reflect color code format in general?
That would remove need for parseInt here, but also I think it's nicer to match 1:1 with actual codes.
Not for this PR though, as they are currently used in a couple places I believe (which is why I wanted to use both until transition is over at least).

astorije · 2017-04-30T13:08:43Z

Not so much actually. Fuzzy matching at the whole completion mechanism level would let you match "Blue" and "Light Blue" when you type "blue" :)

astorije · 2017-04-30T13:10:31Z

Dammit. Will remove when I have a second on laptop.

astorije · 2017-04-30T13:42:41Z

I do, good catch. This solution feels like a crappy hack though. Would you be ok if I add the classes in the CSS file like `.irc-fg03, .irc-fg3 { color: ... }`? Maybe later we can remove the versions without leading zeroes (after converting places that use them) but we could also keep them both permanently for convenience.

xPaw · 2017-04-30T18:22:07Z

.irc-fg03, .irc-fg3 { color: ... }?

That would only look like code smell to me.

astorije · 2017-04-30T19:24:56Z

Why?!

xPaw · 2017-05-06T10:42:18Z

client/js/lounge.js

+			callback(matchingColorCodes);
+		},
+		template(value) {
+			return `<span class="irc-fg${parseInt(value[2], 10)} irc-bg irc-bg${parseInt(value[0], 10)}">${value[1]}</span>`;


Is that an erroneous extra irc-bg?

EDIT: Nope, it's used for styles.

…e-colors Add autocomplete strategy for foreground and background colors

astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Apr 28, 2017

xPaw reviewed Apr 29, 2017

View reviewed changes

astorije force-pushed the astorije/autocomplete-colors branch 2 times, most recently from 98cac12 to f575612 Compare April 30, 2017 10:30

astorije changed the title ~~Add autocomplete strategy for foreground colors~~ Add autocomplete strategy for foreground and background colors Apr 30, 2017

xPaw reviewed Apr 30, 2017

View reviewed changes

xPaw added this to the 2.3.0 milestone Apr 30, 2017

astorije force-pushed the astorije/autocomplete-colors branch from f575612 to 8b16f3d Compare April 30, 2017 11:26

astorije force-pushed the astorije/autocomplete-colors branch from 8b16f3d to 9880ed9 Compare April 30, 2017 11:44

xPaw reviewed Apr 30, 2017

View reviewed changes

Add autocomplete strategy for foreground colors

6d1eef8

astorije force-pushed the astorije/autocomplete-colors branch from 9880ed9 to 695270d Compare April 30, 2017 12:08

xPaw reviewed Apr 30, 2017

View reviewed changes

Add autocomplete strategy for background colors

0981605

astorije force-pushed the astorije/autocomplete-colors branch from 695270d to 0981605 Compare May 1, 2017 00:50

xPaw approved these changes May 1, 2017

View reviewed changes

astorije assigned AlMcKinlay May 1, 2017

xPaw reviewed May 6, 2017

View reviewed changes

xPaw merged commit 57d7616 into master May 6, 2017

xPaw deleted the astorije/autocomplete-colors branch May 6, 2017 10:42

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

Merge pull request thelounge#1109 from thelounge/astorije/autocomplet…

4c1a4a2

…e-colors Add autocomplete strategy for foreground and background colors

AlMcKinlay removed their assignment Mar 12, 2018

Add autocomplete strategy for foreground and background colors #1109

Add autocomplete strategy for foreground and background colors #1109

Conversation

astorije commented Apr 28, 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

xPaw commented Apr 29, 2017

astorije commented Apr 29, 2017

astorije commented Apr 30, 2017

astorije commented Apr 30, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Apr 30, 2017

xPaw commented Apr 30, 2017 • edited Loading

astorije commented Apr 30, 2017

astorije commented Apr 30, 2017

Choose a reason for hiding this comment

astorije Apr 30, 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

Choose a reason for hiding this comment

xPaw commented Apr 30, 2017

astorije commented Apr 30, 2017

xPaw commented Apr 30, 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

astorije commented Apr 30, 2017 via email

astorije commented Apr 30, 2017 via email

astorije commented Apr 30, 2017 via email • edited Loading

xPaw commented Apr 30, 2017

astorije commented Apr 30, 2017 via email

xPaw May 6, 2017 • edited Loading

Choose a reason for hiding this comment

astorije commented Apr 28, 2017 •

edited

Loading

xPaw commented Apr 30, 2017 •

edited

Loading

astorije Apr 30, 2017 •

edited

Loading

astorije commented Apr 30, 2017 via email •

edited

Loading

xPaw May 6, 2017 •

edited

Loading