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

Issue 29519650: Issue 5540 - implement notification (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 3 months ago by saroyanm
Modified:
2 years, 2 months ago
Reviewers:
ire
CC:
juliandoucette, Thomas Greiner
Visibility:
Public.

Patch Set 1 : #

Total comments: 45

Patch Set 2 : #

Total comments: 6

Patch Set 3 : #

Total comments: 4

Patch Set 4 : #

Patch Set 5 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+110 lines, -4 lines) Patch
M locale/en-US/new-options.json View 1 2 3 4 2 chunks +14 lines, -4 lines 0 comments Download
M new-options.html View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
M new-options.js View 1 2 3 4 3 chunks +21 lines, -0 lines 0 comments Download
A skin/delete.svg View 1 chunk +1 line, -0 lines 0 comments Download
M skin/new-options.css View 1 2 3 4 2 chunks +67 lines, -0 lines 0 comments Download

Messages

Total messages: 14
saroyanm
I didn't specify a specific reviewer yet, will do as soon I'll know how occupied ...
2 years, 3 months ago (2017-08-18 22:07:43 UTC) #1
saroyanm
This is ready for the review, @Ire can you please have a look, but prioritize ...
2 years, 2 months ago (2017-08-23 18:58:07 UTC) #2
ire
Thanks Manvel! Here are my first comments. https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-options.json#newcode419 locale/en-US/new-options.json:419: "description": "Notification ...
2 years, 2 months ago (2017-08-24 10:08:45 UTC) #3
saroyanm
Awesome, I think I also have everything here for the next patch. https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-options.json File locale/en-US/new-options.json ...
2 years, 2 months ago (2017-08-24 14:18:27 UTC) #4
saroyanm
https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-options.json#newcode168 locale/en-US/new-options.json:168: "options_whitelist_description": { This ID is duplicated. as noticed here ...
2 years, 2 months ago (2017-08-24 17:54:00 UTC) #5
saroyanm
Thanks for the review Ire. I've uploaded the new patch. https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519650/diff/29525623/locale/en-US/new-options.json#newcode168 ...
2 years, 2 months ago (2017-08-24 18:40:54 UTC) #6
ire
Thanks Manvel. Notes: https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29525623/new-options.html#newcode380 new-options.html:380: <button data-action="hide-notification" id="hide-notification"></button> On 2017/08/24 14:18:25, ...
2 years, 2 months ago (2017-08-25 09:59:46 UTC) #7
saroyanm
Thanks Ire, I've addressed your comments and a new patch is uploaded. https://codereview.adblockplus.org/29519650/diff/29526765/new-options.html File new-options.html ...
2 years, 2 months ago (2017-08-25 11:04:19 UTC) #8
ire
https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js#newcode1004 new-options.js:1004: function showNotification(text) On 2017/08/25 11:04:19, saroyanm wrote: > On ...
2 years, 2 months ago (2017-08-25 12:10:08 UTC) #9
saroyanm
https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29519650/diff/29526765/new-options.js#newcode1004 new-options.js:1004: function showNotification(text) On 2017/08/25 12:10:07, ire wrote: > On ...
2 years, 2 months ago (2017-08-25 12:22:57 UTC) #10
ire
https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html#newcode380 new-options.html:380: <button id="hide-notification" data-action="hide-notification" aria-label="Close Notification"></button> On 2017/08/25 12:22:56, saroyanm ...
2 years, 2 months ago (2017-08-25 12:43:45 UTC) #11
saroyanm
https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519650/diff/29527656/new-options.html#newcode380 new-options.html:380: <button id="hide-notification" data-action="hide-notification" aria-label="Close Notification"></button> On 2017/08/25 12:43:44, ire ...
2 years, 2 months ago (2017-08-25 13:22:09 UTC) #12
saroyanm
Rebased
2 years, 2 months ago (2017-08-25 13:49:29 UTC) #13
ire
2 years, 2 months ago (2017-08-25 14:35:30 UTC) #14
LGTM
Sign in to reply to this message.

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