Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(177)

Issue 29583620: Issue 5354 - Adds handling for notifications on Opera (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by Jon Sonesen
Modified:
1 year, 10 months ago
Reviewers:
Sebastian Noack
Visibility:
Public.

Description

Issue 5354 - Adds handling for notifications on Opera

Patch Set 1 #

Total comments: 5

Patch Set 2 : adress comments p1, rebase #

Total comments: 2

Patch Set 3 : address p2 comment #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -1 line) Patch
M lib/notificationHelper.js View 1 2 1 chunk +11 lines, -1 line 1 comment Download

Messages

Total messages: 7
Jon Sonesen
1 year, 11 months ago (2017-10-19 21:56:38 UTC) #1
Jon Sonesen
https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHelper.js#newcode211 lib/notificationHelper.js:211: delete options.buttons; perhaps it is better to move this ...
1 year, 11 months ago (2017-10-19 21:59:17 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHelper.js#newcode209 lib/notificationHelper.js:209: Adding buttons to notifications is not supported.`) I would ...
1 year, 11 months ago (2017-10-21 21:04:28 UTC) #3
Jon Sonesen
https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHelper.js#newcode209 lib/notificationHelper.js:209: Adding buttons to notifications is not supported.`) On 2017/10/21 ...
1 year, 10 months ago (2017-10-30 22:41:41 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29583620/diff/29592648/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29583620/diff/29592648/lib/notificationHelper.js#newcode215 lib/notificationHelper.js:215: if (activeNotification.type != "question") You can just check both ...
1 year, 10 months ago (2017-10-30 22:46:43 UTC) #5
Jon Sonesen
https://codereview.adblockplus.org/29583620/diff/29592648/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29583620/diff/29592648/lib/notificationHelper.js#newcode215 lib/notificationHelper.js:215: if (activeNotification.type != "question") On 2017/10/30 22:46:43, Sebastian Noack ...
1 year, 10 months ago (2017-11-01 18:21:44 UTC) #6
Sebastian Noack
1 year, 10 months ago (2017-11-01 18:45:03 UTC) #7
LGTM except for one typo. Feel free to go ahead pushing the change once you
fixed the typo. There is no need for me to review it again (if you don't change
anything else).

https://codereview.adblockplus.org/29583620/diff/29594666/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29583620/diff/29594666/lib/notificationHel...
lib/notificationHelper.js:211: // Opera does not support the addtition of
buttons to notifications.
Typo: addtition
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5