Add custom highlights. #580

AlMcKinlay · 2016-01-13T21:14:00Z

A couple of issues with this PR still.

I think that highlights really need to be saved across browsers. I'm not sure what others think.
I need to rearrange the settings a little bit still.

But thought I'd stick it up now to see what people think.

astorije · 2016-01-20T05:57:45Z

client/index.html

 								<label class="opt">
 								<input id="badge" type="checkbox" name="badge">
 								Enable badge
 								</label>
-							</div>
-							<div class="col-sm-12">


Why did you remove these?

So, I've changed slightly how the settings page is organised, because otherwise having an input with a label ended up making the other side have a blank line.

Will post screenshots later.

Wouldn't that be better to do in a further PR? I mean, the rest of the PR is definitely wanted (granted, after some more back-and-forth) but UI stuff is usually more opinionated and might delay, even for a few days. I prefer small and split PRs that can be merged quickly, avoiding author frustration :-)
But your call if you think it cannot be split.

I've changed it back to what it was before and it works like I would have expected it to, which is odd. So I've reverted this.

Edit: actually, was editing the wrong repository. The problem with keeping them on col-sm-12 is that we end up with this:

If we do it the way I've dne it, we get this:

If you think that there will be lots of discussion over the new format, then I'll take it out. But I really don't like how it looks in the first place.

I don't have a strong opinion on this. (However I find it ugly whatever the solution but that's not your fault)

I'm fine with that so you can keep your change, but at some point, when we properly discuss configuration (or even before that), a UX/UI pass on the settings page will be much appreciated!

Yeah, I think both of them are ugly, but I prefer it after my change. So if no one else has a big objection, I'll keep that in this PR.

Yeah, we really should look at the UI/UX of the settings in general. Could be done in conjunction with adding server syncing (we could have optional overrides for individual clients for example).

But let's leave that to the other discussion. I'll add an issue if no one else has.

astorije · 2016-01-20T06:21:59Z

Hey @YaManicKill, thanks for this great contribution!

I think that highlights really need to be saved across browsers. I'm not sure what others think.

I wonder. It's probably nice to have a way to set them differently on desktop and mobile (although mobiles don't support notifications very well so far), but it might also be a pain to set them individually on all devices/browsers.
All other attempts to set these will result in convoluted code, until we come up with a better plan for setting up persistent settings with potentially per-channel, per-device settings, ... while keeping something user-friendly (I want to avoid thousands of settings to customize the world on a tool that just looks nice and easy)!
In the meantime, this is good before it gets better, so I'm okay with this solution :-) I wouldn't worry about this item for this PR, really.

I need to rearrange the settings a little bit still.

What are you planning to do exactly?

Can't wait to have this merged in! :-)

A nice idea for a future PR could be a tag input such as this but this is waaay off the topic of this PR!

AlMcKinlay · 2016-01-20T15:55:28Z

In the meantime, this is good before it gets better, so I'm okay with this solution :-) I wouldn't worry about this item for this PR, really.

Alrighty. I think it's something we should probably look into allowing certain settings to copy across browsers, but I'm happy to keep this without that just now, as it's not that difficult to copy them over different machines manually. But not ideal in the long run.

What are you planning to do exactly?

Just I'm not happy with the placement of the highlights input. I'll post screenshots probably tomorrow once I've recovered from my business trip.

astorije · 2016-01-21T05:01:51Z

I think it's something we should probably look into allowing certain settings to copy across browsers, but I'm happy to keep this without that just now, as it's not that difficult to copy them over different machines manually. But not ideal in the long run.

Agreed, but you live to fight for another day :-) Also, this needs some design and decision making (to make sure we have the best possible UX), and I don't want to delay this PR for something not directly related. But clearly I agree with you!

astorije · 2016-01-21T05:02:31Z

Just I'm not happy with the placement of the highlights input. I'll post screenshots probably tomorrow once I've recovered from my business trip.

Awesome! I can't wait but no pressure, I can't promise I'll be always be able to review on time anyway.

AlMcKinlay · 2016-01-22T21:26:32Z

Ok, I've updated with the changes you suggested, let me know if you think it's in a mergable state now :-)

xPaw · 2016-01-23T08:49:17Z

client/js/shout.js

@@ -382,6 +386,20 @@ $(function() {
 		}
 	}

+	if ($.cookie("highlights") !== null) {


Use the existing settings cookie object, it will be easier to switch to another storage method in the future.

Good point, what do you think @YaManicKill?

Yes, I think this would be the best way to do things. I've done this in #603, let me know what you think about how it's done there. I can just do it the same way.

astorije · 2016-01-23T23:42:06Z

Hey @YaManicKill, great, that looks very good! One minor comment away, but nothing big.
Also, #596 will probably (at least it should) merged first, and it will affect this so you'll have to rebase. Sorry about that!

AlMcKinlay · 2016-01-24T07:12:12Z

Yeah, take a look at #603 as well, I've changed how I'm doing the settings for that one, which might well be better to do with this one. Like why @xPaw says. Let me know what you think when you have time.

AlMcKinlay · 2016-02-02T10:56:58Z

I'll update this this weekend with the settings change I made in #603 (not doing this in a seperate cookie) and we can hopefully get it merged before the fork.

AlMcKinlay · 2016-02-04T19:56:31Z

Alright, that should be this in a good state to merge, I think. I've changed how we do the saving of the setting, so it saves in the same cookie, and now the saving and reading from the cookie works for more than just checkboxes.

Let me know if there's anything else, but I think this should be good.

astorije · 2016-02-15T00:57:16Z

@YaManicKill, care to move this to https://github.com/thelounge/lounge? Ultimately this could be a package in itself, but since it's already in a working condition, why not.

AlMcKinlay · 2016-02-15T08:29:35Z

Oh yeah, of course. I'll get this ported over this evening.

astorije reviewed Jan 20, 2016
View reviewed changes

astorije self-assigned this Jan 20, 2016

astorije added the enhancement label Jan 20, 2016

xPaw reviewed Jan 23, 2016
View reviewed changes

AlMcKinlay mentioned this pull request Jan 24, 2016

Settings not saved for user #542

Open

Add custom highlights.

9e135c3

shazeline mentioned this pull request Jul 30, 2016

Custom highlights thelounge/thelounge#160

Closed

astorije removed their assignment Mar 11, 2018

AlMcKinlay closed this Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom highlights. #580

Add custom highlights. #580

AlMcKinlay commented Jan 13, 2016

astorije Jan 20, 2016

AlMcKinlay Jan 20, 2016

astorije Jan 21, 2016

AlMcKinlay Jan 21, 2016

astorije Jan 22, 2016

AlMcKinlay Jan 22, 2016

astorije commented Jan 20, 2016

AlMcKinlay commented Jan 20, 2016

astorije commented Jan 21, 2016

astorije commented Jan 21, 2016

AlMcKinlay commented Jan 22, 2016

xPaw Jan 23, 2016

astorije Jan 23, 2016

AlMcKinlay Jan 24, 2016

astorije commented Jan 23, 2016

AlMcKinlay commented Jan 24, 2016

AlMcKinlay commented Feb 2, 2016

AlMcKinlay commented Feb 4, 2016

astorije commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016

Add custom highlights. #580

Add custom highlights. #580

Conversation

AlMcKinlay commented Jan 13, 2016

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 Jan 20, 2016

AlMcKinlay commented Jan 20, 2016

astorije commented Jan 21, 2016

astorije commented Jan 21, 2016

AlMcKinlay commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Jan 23, 2016

AlMcKinlay commented Jan 24, 2016

AlMcKinlay commented Feb 2, 2016

AlMcKinlay commented Feb 4, 2016

astorije commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016