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

Issue 29609587: Issue 6031 - Implement Acceptable Ads notification (Closed)

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

Description

Issue 6031 - Implement Acceptable Ads notification

Patch Set 1 #

Total comments: 21

Patch Set 2 : Rebased #

Patch Set 3 : Addressed Thomas comments #

Total comments: 23

Patch Set 4 : Rebased #

Patch Set 5 : Addressed comments #

Total comments: 6

Patch Set 6 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+138 lines, -5 lines) Patch
M background.js View 1 chunk +1 line, -0 lines 0 comments Download
M desktop-options.html View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M desktop-options.js View 1 2 3 4 5 6 chunks +40 lines, -1 line 0 comments Download
M locale/en_US/desktop-options.json View 1 2 3 4 5 1 chunk +32 lines, -0 lines 0 comments Download
M skin/desktop-options.css View 1 2 3 6 chunks +50 lines, -4 lines 0 comments Download
M skin/icons/delete.svg View 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 10
saroyanm
@Thomas while Ire will be on vacation, can you please have a look, considering the ...
2 years ago (2017-11-15 17:15:09 UTC) #1
Thomas Greiner
https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.html#newcode100 desktop-options.html:100: <div id="tracking-notification"> Suggestion: The naming is inconsistent. Here are ...
2 years ago (2017-11-16 19:25:06 UTC) #2
saroyanm
Thanks Thomas! New patch is ready for the review. https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.html#newcode100 desktop-options.html:100: ...
2 years ago (2017-11-17 16:10:40 UTC) #3
saroyanm
https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/desktop-options.json File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/desktop-options.json#newcode72 locale/en_US/desktop-options.json:72: "message": "We noticed you have both <strong>'$easy-privacy$'</strong> and '<strong>$acceptable-ads$</strong>' ...
2 years ago (2017-11-17 16:13:26 UTC) #4
saroyanm
https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#newcode1379 desktop-options.js:1379: if (!value) Does it make sense to check against ...
2 years ago (2017-11-17 16:45:14 UTC) #5
Thomas Greiner
https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-options.css File skin/desktop-options.css (right): https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-options.css#newcode635 skin/desktop-options.css:635: background-color: #fefbe3; On 2017/11/17 16:10:39, saroyanm wrote: > On ...
2 years ago (2017-11-20 18:36:06 UTC) #6
saroyanm
Sorry that it took longer to address the comments. New patch uploaded. Ready for the ...
2 years ago (2017-11-21 14:58:33 UTC) #7
Thomas Greiner
https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#newcode1379 desktop-options.js:1379: if (!value) On 2017/11/21 14:58:32, saroyanm wrote: > I ...
2 years ago (2017-11-21 17:17:30 UTC) #8
saroyanm
Thanks Thomas New patch uploaded. https://codereview.adblockplus.org/29609587/diff/29613759/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29609587/diff/29613759/desktop-options.js#newcode1380 desktop-options.js:1380: E("acceptable-ads").classList.add("show-warning"); On 2017/11/21 17:17:29, ...
2 years ago (2017-11-21 18:17:35 UTC) #9
Thomas Greiner
2 years ago (2017-11-21 19:04:32 UTC) #10
LGTM
Sign in to reply to this message.

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