search

erming / shout

Deprecated. See fork @ https://github.com/thelounge
MIT License
3.62k stars 272 forks source link

Add custom highlights. #580

Closed AlMcKinlay closed 4 years ago

AlMcKinlay commented 8 years ago

A couple of issues with this PR still.

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

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

astorije commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

astorije commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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