-
-
Notifications
You must be signed in to change notification settings - Fork 832
Add option to disable typing notifications #3494
Conversation
Re sign-off: I believe as per the contributing doc it has to be name and e-mail address. Also your name has to fit within:
|
That's alright. I fixed it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First step here I think is a product decision on whether we want this option.
Code-wise, I'd be a lot happier about merging this if it came with a test, otherwise it's liable to just end up getting broken at some point down the line and we won't notice.
I don't see any reason why this option would be unwanted. I've heard several people wishing for it because the typing notifications are distracting to them. I just find them unnecessary. Since this options defaults to "show typing notifications" anyway, I don't understand the problem of including it. However, this decision is not up to me. |
I'd say given Tom labelled it as a feature with a priority it seems like having it behind a default-off setting should be fine |
supportedLevels: LEVELS_ACCOUNT_SETTINGS, | ||
displayName: _td("Show typing notifications"), | ||
default: true, | ||
invertedSettingName: 'dontShowTypingNotifications', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be here - there's no legacy setting to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring solely to line 231? It's a bit unclear from how github marked it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, 231. The invertedSettingName
is a backwards compatibility thing, but there's nothing to be backwards compatible with here.
It's also been a year - we should double check. |
Resolves this riot-im issue.
Signed-off-by: Lukas Schwarz ghub@posteo.se