-
Notifications
You must be signed in to change notification settings - Fork 337
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
Add notifications #1352
Add notifications #1352
Conversation
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.
UI looks good to me!
365cddb
to
11e7615
Compare
It should work! I'm not sure what it does if the to-be-notified device is asleep/unresponsive |
response.status(200).json({publicKey: vapid.publicKey}); | ||
}); | ||
|
||
PushController.post('/register', async (request, response) => { |
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.
General question: Is there a way to deregister?
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.
If someone revokes the notification permission it will be removed in the sendNotification's error handler
src/models/PushSubscriptions.js
Outdated
} | ||
async load() { | ||
this.subscriptions = await Database.getPushSubscriptions(); | ||
} |
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.
Missing a closing } here.
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.
Woops, meant to delete this file
*/ | ||
function NotificationEffectBlock(ruleArea, onPresentationChange, onRuleUpdate) { | ||
RulePartBlock.call(this, ruleArea, onPresentationChange, onRuleUpdate, | ||
'Notification', '/optimized-images/rule-icons/notification.svg'); |
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.
Line is too long.
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 is very cool. My only nitpick is that the if/while selector is still enabled when setting a notification, but I can't actually change it.
Codecov Report
@@ Coverage Diff @@
## master #1352 +/- ##
==========================================
+ Coverage 75.3% 75.46% +0.16%
==========================================
Files 123 125 +2
Lines 5989 6225 +236
Branches 842 885 +43
==========================================
+ Hits 4510 4698 +188
- Misses 1306 1353 +47
- Partials 173 174 +1
Continue to review full report at Codecov.
|
Yeah, the if/while thing is just a drawback of a different part of the rules engine that I can fix in a different PR |
Fixes #1295