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

Issue 29326181: Issue 3022 - Implemented new notification type for normal messages (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 3 months ago by Thomas Greiner
Modified:
4 years, 3 months ago
Reviewers:
Sebastian Noack
CC:
Felix Dahlke
Visibility:
Public.

Description

In addition to what's specified in the ticket description, this review also contains the following fixes: 1. Remove dead "question"-type notification code from icon popup because the `#notification-question` no longer exists anyway. 2. If a notification specifies an unknown type, don't try to animate the icon because this currently results in errors when trying to fetch the icon images because they don't exist.

Patch Set 1 #

Total comments: 2

Patch Set 2 : Centralized mapping information for types #

Total comments: 3

Patch Set 3 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+27 lines, -26 lines) Patch
M lib/notificationHelper.js View 1 2 4 chunks +23 lines, -2 lines 1 comment Download
M notification.js View 1 3 chunks +4 lines, -24 lines 0 comments Download

Messages

Total messages: 9
Thomas Greiner
4 years, 3 months ago (2015-09-09 13:20:15 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29326181/diff/29326182/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29326181/diff/29326182/lib/notificationHelper.js#newcode45 lib/notificationHelper.js:45: let animateIcon = (activeNotification.type == "critical" I wonder whether ...
4 years, 3 months ago (2015-09-09 13:35:36 UTC) #2
Thomas Greiner
Just a sidenote: I think that, in the long term, it may make sense to ...
4 years, 3 months ago (2015-09-09 15:32:36 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHelper.js#newcode27 lib/notificationHelper.js:27: let displayMethods = { Please use Object.create(null) for objects ...
4 years, 3 months ago (2015-09-09 17:38:58 UTC) #4
Thomas Greiner
On 2015/09/09 17:38:58, Sebastian Noack wrote: > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHelper.js > File lib/notificationHelper.js (right): > > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHelper.js#newcode27 ...
4 years, 3 months ago (2015-09-10 11:11:39 UTC) #5
Sebastian Noack
On 2015/09/10 11:11:39, Thomas Greiner wrote: > It appears that all these comments are personal ...
4 years, 3 months ago (2015-09-10 11:36:02 UTC) #6
Thomas Greiner
On 2015/09/10 11:36:02, Sebastian Noack wrote: > On 2015/09/10 11:11:39, Thomas Greiner wrote: > > ...
4 years, 3 months ago (2015-09-10 13:56:06 UTC) #7
Sebastian Noack
On 2015/09/10 13:56:06, Thomas Greiner wrote: > Thanks, I really appreciate the feedback. I'm usually ...
4 years, 3 months ago (2015-09-10 13:58:23 UTC) #8
Sebastian Noack
4 years, 3 months ago (2015-09-11 13:15:46 UTC) #9
Message was sent while issue was closed.
https://codereview.adblockplus.org/29326181/diff/29327139/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29326181/diff/29327139/lib/notificationHel...
lib/notificationHelper.js:256: exports.shouldDisplay = shouldDisplay;
I just realized that this way shouldDisplay is documented as static member
rather than a method of the module:
https://adblockplus.org/jsdoc/adblockpluschrome/module-notificationHelper.html

Also see: http://usejsdoc.org/howto-commonjs-modules.html

Mind writing a follow-up patch to fix that? I'd go for:

  let shouldDisplay =
  /**
   * ...
   */
  exports.shouldDisplay = function()
  {
    ...
  };

We already do it that way in other modules.
Sign in to reply to this message.

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