Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure notifications by type #422

Merged
merged 7 commits into from
Jun 2, 2020

Conversation

dideler
Copy link
Contributor

@dideler dideler commented May 20, 2020

I have most notifications on all my devices reduced to a minimum. To reduce notification fatigue, I'm only interested in important notifications. For Lepton, I'm only interested in exceptional notifications, i.e. where the expected result hasn't occurred. I think it's especially important for macOS because these notifications cannot be force closed.

Tasks

  • Add structured configuration for notifications by type so it's extendable
  • By default, all notification types are turned on
  • Document notification configuration options
  • Deprecate or immediately remove disableNotification option
    • No longer needed, can achieve similar behaviour with config for notification types, or can already block all app notifications at the OS level
    • Wasn't documented anywhere except in #377 Disable Notifications #382, so may only affect a small number of users

@dideler
Copy link
Contributor Author

dideler commented May 20, 2020

BTW haven't yet soft-deprecated the disableNotification option because I'd like to get your thoughts on that first, and because I don't know where to put the notice besides the release notes since the option was never documented.

@hackjutsu
Copy link
Owner

Hi @dideler , it's my bad to forget mentioning the disableNotification here. We can move the it to README or Wiki if that works better.

I like your approach to distinguish success and failure notification and their configurations. We can deprecate the disableNotification field and use your solution instead. The impact should be minimal since we didn't document this field 😅 cc the original PR author @alexandreamadocastro for awareness.

@dideler
Copy link
Contributor Author

dideler commented May 28, 2020

Hey @hackjutsu, I'll go ahead and remove disableNotification now to simplify things.

I noticed there's already a project wiki, but I don't have permissions to edit it. In order to document the config changes, could you either change the wiki permissions, or alternatively, we can move the config docs somewhere in the code repo and then I won't be blocked.

@hackjutsu
Copy link
Owner

@dideler The wiki permission is updated. Let me if it works.

The previous setting "disableNotification" isn't extendable.
There's no hierarchy to group notification settings. New notification
settings would end up as a top-level config, which isn't consistent with
existing configuration structure.
By classifying the type of notification, we can treat them differently.
For now it's only whether to show the notification type, but in the
future can also do things such as styling, formatting, content, etc.
@dideler dideler force-pushed the configure-notifications-by-type branch from 5c291de to 557a160 Compare June 1, 2020 18:37
@dideler
Copy link
Contributor Author

dideler commented Jun 1, 2020

@hackjutsu wonderful, thank you! I've created a wiki page, moved over the existing content, added missing options, added the new notifications option, and updated the README.

I think this merge request is good to go now.

@hackjutsu
Copy link
Owner

Cool, thanks for the contribution!

@hackjutsu hackjutsu merged commit 94f5dfc into hackjutsu:master Jun 2, 2020
@dideler dideler deleted the configure-notifications-by-type branch June 8, 2020 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants