Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Implement notifications for all messages #540

Merged
merged 1 commit into from
Jan 10, 2016

Conversation

nicolashohm
Copy link
Contributor

Implement option to get notifications also for message not just for mentions

@nicolashohm nicolashohm changed the title Implement option to get notifications also for messages Implement notifications for all messages Nov 14, 2015
@astorije
Copy link
Collaborator

Hi @nickel715, thanks a lot for your contribution!

This seems like a fine PR, I'll take ownership but I'll have to try it out before giving my 👍.

@erming, @JocelynDelalande, @floogulinc, what do we think about this feature overall? To me it doesn't seem like overhead on either the code or the UI, and people might want this anyway.

In the future, I'd like to see all of this dealt by the plugin system (hint hint, help needed on the plugin system!). How we handle with this in details is however TBD.

@astorije astorije self-assigned this Nov 23, 2015
@floogulinc
Copy link
Collaborator

I think if we add this option, it should be able to be toggled on a per channel basis. There are some channels (more private ones) that I would like to be notified for any message, but others, large channels with constant conversation, it would just be spam.

@@ -347,6 +347,7 @@ $(function() {
part: true,
thumbnails: true,
quit: true,
notifyMessage: false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would reword as "notifyAllMessages"

@JocelynDelalande
Copy link
Collaborator

Thanks for contrib @nickel715

Read and tested, appart the naming issue I commented in the diff, it's ok for me (dare to fix your PR @nickel715 ?)

Unlike @floogulinc I think this can go as-is into shout. Per-chan toggle could go to another PR if someone wants to code it ; what you suggest is clearly a valid feature @floogulinc .

@floogulinc
Copy link
Collaborator

@JocelynDelalande as long as it's an option, and off by default, I'm totally fine adding it as-is. 👍

Unreleased
==========

* Implement option to get notifications also for messages not just for mentions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't really think we do this. We add the full changelog upon release of a new version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, should i revert the addition in changelog.md?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickel715 if you're already making a commit for something else, then you could. It's not a big concern.

@nicolashohm
Copy link
Contributor Author

@JocelynDelalande thanks for review.
I agree with you, per-chan toggle could go to another feature.
Rewording of notifyMessage sounds good, it's more precise. I will change it.

@MaxLeiter
Copy link
Contributor

bump? Would love this

@nicolashohm
Copy link
Contributor Author

@MaxLeiter thanks for the reminder, now alle comments are fixed :)

@JocelynDelalande
Copy link
Collaborator

@nickel715 awesome, thanks :) 👍

@astorije @floogulinc ok for you ?

@JocelynDelalande
Copy link
Collaborator

@nickel715 could you just squash your commits into one please ?

@astorije
Copy link
Collaborator

Hi @nickel715, thanks again for this!
I am also a 👍 on this but as @JocelynDelalande, I'd like you to squash these 2 commits into 1.
If you are not familiar with the process, follow these steps and be done with it:

  1. Checkout to your branch, then run git rebase -i HEAD~2.
  2. This will open a text editor and mark the second commit at a fixup (replace pick on the second line with f). Save and exit the editor.
  3. Push with git push -f

Once you have done that, comment on this thread so that I get notified, and I'll merge this! It will then ship with the next release, hopefully as soon as possible.

Implement option to get notifications also for message not just for mentions

Resolves: erming#446
@nicolashohm
Copy link
Contributor Author

@astorije done. sorry for the delay

@astorije
Copy link
Collaborator

Awesome, thanks again!

astorije added a commit that referenced this pull request Jan 10, 2016
Implement notifications for all messages
@astorije astorije merged commit ab2dc3e into erming:master Jan 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants