Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29712664: Issue 6432 - Hide remove button for additional filter lists (Closed)

Created:
March 1, 2018, 9:43 p.m. by saroyanm
Modified:
March 12, 2018, 2:24 p.m.
CC:
kzar
Visibility:
Public.

Description

Issue 6432 - Hide remove button for additional filter lists

Patch Set 1 #

Total comments: 18

Patch Set 2 : Addressed Dave's comments #

Patch Set 3 : Addressed Thomas's comments #

Total comments: 1

Patch Set 4 : Addressed Jeen's suggestion in the ticket #

Total comments: 10

Patch Set 5 : Addressed Thomas's comments #

Total comments: 5

Patch Set 6 : Rebased #

Patch Set 7 : Version with no callback hell #

Patch Set 8 : Keeping callback hell #

Total comments: 6

Patch Set 9 : Reverted the colors back #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -23 lines) Patch
M README.md View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M background.js View 1 2 5 chunks +16 lines, -8 lines 0 comments Download
M css/desktop-options.scss View 1 2 3 4 5 8 3 chunks +16 lines, -0 lines 0 comments Download
M desktop-options.html View 1 2 3 4 5 6 4 chunks +6 lines, -6 lines 0 comments Download
M js/desktop-options.js View 1 2 3 4 5 6 7 3 chunks +25 lines, -9 lines 0 comments Download
M skin/icons/checkbox.svg View 1 2 3 4 8 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 21
saroyanm
Initial implementation with a questions. @Thomas: can you please have a look when you have ...
March 1, 2018, 9:52 p.m. (2018-03-01 21:52:34 UTC) #1
a.giammarchi
On 2018/03/01 21:52:34, saroyanm wrote: > @Andrea: Is there an easy way to disable the ...
March 1, 2018, 10:14 p.m. (2018-03-01 22:14:21 UTC) #2
a.giammarchi
btw, I don't have answers for your question in code but otherwise these changes look ...
March 2, 2018, 9:19 a.m. (2018-03-02 09:19:09 UTC) #3
kzar
https://codereview.adblockplus.org/29712664/diff/29712665/background.js File background.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newcode112 background.js:112: additional_subscriptions: (params.additionalSubscriptions) ? ["https://easylist-downloads.adblockplus.org/easylistgermany+easylist.txt"] : [], Nit: The paranthesis ...
March 2, 2018, 9:57 a.m. (2018-03-02 09:57:01 UTC) #4
Thomas Greiner
https://codereview.adblockplus.org/29712664/diff/29712665/background.js File background.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newcode71 background.js:71: additionalSubscriptions: false, Detail: Please also mention this new parameter ...
March 2, 2018, 11:33 a.m. (2018-03-02 11:33:36 UTC) #5
saroyanm
Ready for review. https://codereview.adblockplus.org/29712664/diff/29712665/background.js File background.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newcode71 background.js:71: additionalSubscriptions: false, On 2018/03/02 11:33:35, Thomas ...
March 2, 2018, 1:50 p.m. (2018-03-02 13:50:29 UTC) #6
saroyanm
On 2018/03/01 22:14:21, a.giammarchi wrote: > On 2018/03/01 21:52:34, saroyanm wrote: > > @Andrea: Is ...
March 2, 2018, 1:53 p.m. (2018-03-02 13:53:12 UTC) #7
saroyanm
https://codereview.adblockplus.org/29712664/diff/29713577/skin/desktop-options.css File skin/desktop-options.css (right): https://codereview.adblockplus.org/29712664/diff/29713577/skin/desktop-options.css#newcode1080 skin/desktop-options.css:1080: display: none !important; I had to use import because ...
March 2, 2018, 2 p.m. (2018-03-02 14:00:15 UTC) #8
saroyanm
Addressed Jeen's suggetion in the ticket, about disabled checkbox.
March 2, 2018, 4:32 p.m. (2018-03-02 16:32:53 UTC) #9
Thomas Greiner
https://codereview.adblockplus.org/29712664/diff/29712665/background.js File background.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newcode107 background.js:107: notifications_ignoredcategories: (params.showNotificationUI) ? ["*"] : [], Detail: This change ...
March 2, 2018, 5 p.m. (2018-03-02 17:00:03 UTC) #10
saroyanm
https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.html#newcode89 desktop-options.html:89: <button data-preconfigured="hide" data-action="toggle-remove-subscription" role="checkbox" class="control icon"></button> On 2018/03/02 17:00:02, ...
March 2, 2018, 5:36 p.m. (2018-03-02 17:36:32 UTC) #11
kzar
(Moving myself back to Cc) https://codereview.adblockplus.org/29712664/diff/29712665/background.js File background.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newcode112 background.js:112: additional_subscriptions: (params.additionalSubscriptions) ? ["https://easylist-downloads.adblockplus.org/easylistgermany+easylist.txt"] ...
March 5, 2018, 1:23 p.m. (2018-03-05 13:23:21 UTC) #12
kzar
March 5, 2018, 1:23 p.m. (2018-03-05 13:23:45 UTC) #13
Thomas Greiner
https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.html#newcode89 desktop-options.html:89: <button data-preconfigured="hide" data-action="toggle-remove-subscription" role="checkbox" class="control icon"></button> What about reversing ...
March 5, 2018, 2:52 p.m. (2018-03-05 14:52:11 UTC) #14
saroyanm
Ready for the review. https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.html#newcode89 desktop-options.html:89: <button data-preconfigured="hide" data-action="toggle-remove-subscription" role="checkbox" class="control ...
March 6, 2018, 3:09 p.m. (2018-03-06 15:09:53 UTC) #15
Thomas Greiner
https://codereview.adblockplus.org/29712664/diff/29713622/skin/icons/checkbox.svg File skin/icons/checkbox.svg (right): https://codereview.adblockplus.org/29712664/diff/29713622/skin/icons/checkbox.svg#newcode13 skin/icons/checkbox.svg:13: <rect width="17" height="17" fill="#b3b3b3" stroke="#b3b3b3" rx="2" x=".5" y=".5" /> ...
March 7, 2018, 5:45 p.m. (2018-03-07 17:45:39 UTC) #16
saroyanm
Thanks Thomas, this is ready for another review round. https://codereview.adblockplus.org/29712664/diff/29713622/skin/icons/checkbox.svg File skin/icons/checkbox.svg (right): https://codereview.adblockplus.org/29712664/diff/29713622/skin/icons/checkbox.svg#newcode13 skin/icons/checkbox.svg:13: ...
March 8, 2018, 6:03 p.m. (2018-03-08 18:03:37 UTC) #17
Thomas Greiner
https://codereview.adblockplus.org/29712664/diff/29715733/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712664/diff/29715733/js/desktop-options.js#newcode244 js/desktop-options.js:244: getPref("additional_subscriptions", (additionalSubscriptions) => On 2018/03/08 18:03:36, saroyanm wrote: > ...
March 8, 2018, 7:28 p.m. (2018-03-08 19:28:19 UTC) #18
saroyanm
https://codereview.adblockplus.org/29712664/diff/29717614/css/desktop-options.scss File css/desktop-options.scss (right): https://codereview.adblockplus.org/29712664/diff/29717614/css/desktop-options.scss#newcode277 css/desktop-options.scss:277: background-color: #b3b3b3; On 2018/03/08 19:28:19, Thomas Greiner wrote: > ...
March 9, 2018, 11:23 a.m. (2018-03-09 11:23:29 UTC) #19
saroyanm
Changing the Stroke color for the "checked disabled" button introduces a distortion which needs to ...
March 9, 2018, 11:28 a.m. (2018-03-09 11:28:03 UTC) #20
Thomas Greiner
March 9, 2018, 5:24 p.m. (2018-03-09 17:24:23 UTC) #21
On 2018/03/09 11:28:03, saroyanm wrote:
> Changing the Stroke color for the "checked disabled" button introduces a
> distortion which needs to be fixed and tested crossbrowser, so I reverted the
> colors back(made consistent with old disabled color) to do that separately
after
> we have a final proposal for the both disabled states introduced in the specs.

Interesting, yeah, let's tackle this separately then.

LGTM

https://codereview.adblockplus.org/29712664/diff/29717614/css/desktop-options...
File css/desktop-options.scss (right):

https://codereview.adblockplus.org/29712664/diff/29717614/css/desktop-options...
css/desktop-options.scss:277: background-color: #b3b3b3;
On 2018/03/09 11:23:28, saroyanm wrote:
> On 2018/03/08 19:28:19, Thomas Greiner wrote:
> > Detail: Isn't this redundant since we're setting `fill="#b3b3b3"` in the SVG
> > anyway?
> 
> It is not because we do not have dedicated disable state for the Accceptable
Ads
> button, this is something make sense to introduce after we update the specs.
> 
> This implementation is a hack in order to not introduce a new button, this is
> something that needs to be change as part of Jeens change to the specs.
> 
> I'll change this color back to #ccc not to block this review with the color
> discussion.

Acknowledged.

https://codereview.adblockplus.org/29712664/diff/29717614/skin/icons/checkbox...
File skin/icons/checkbox.svg (right):

https://codereview.adblockplus.org/29712664/diff/29717614/skin/icons/checkbox...
skin/icons/checkbox.svg:13: <rect width="17" height="17" fill="#b3b3b3"
stroke="#b3b3b3" rx="2" x=".5" y=".5" />
On 2018/03/09 11:23:28, saroyanm wrote:
> On 2018/03/08 19:28:19, Thomas Greiner wrote:
> > According to Jeen's suggestion only the border should be `#B3B3B3` while the
> > background should be `#CCC`.
> > 
> > She also mentioned using `#B3B3B3` for the text but that particular change
may
> > be tricky so I'd say we can ignore that one for now and apply that later
when
> > the spec gets updated.
> 
> This is getting, complicated. changing the fill for this item to "#ccc",
> distorts the button, I'll change this back to use #ccc for both states, to be
> consistent with old Acceptable Ads color and apply new changes as soon we have
> the final design update.

Acknowledged.

Powered by Google App Engine
This is Rietveld