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. |
DescriptionIssue 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 #
MessagesTotal messages: 21
Initial implementation with a questions. @Thomas: can you please have a look when you have a chance. @Andrea: Is there an easy way to disable the eslisting during development, without modifying package.json ? https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.... js/desktop-options.js:243: else if (this == collections.filterLists) I wonder if I also should consider the case of removing checkbox from the Privacy & Security section for the Block additional tracking and Block social media icons tracking, when matched ? Should we still should allow changing or removing the language subscriptions from the General tab ?
On 2018/03/01 21:52:34, saroyanm wrote:
> @Andrea: Is there an easy way to disable the eslisting during development,
> without modifying package.json ?
not sure what you need to disable it for, but one little hack I use from time to
time is to `eval("debugger;")` which AFAIK is allowed by our rules.
Would that help?
btw, I don't have answers for your question in code but otherwise these changes look good.
https://codereview.adblockplus.org/29712664/diff/29712665/background.js File background.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newco... background.js:112: additional_subscriptions: (params.additionalSubscriptions) ? ["https://easylist-downloads.adblockplus.org/easylistgermany+easylist.txt"] : [], Nit: The paranthesis around params.additionalSubscriptions seem pointless. Nit: This line is way too long, it passes linting since it contains a URL but the purpose of that exception is to allow long URLs which would have to be split up otherwise. How about having a few constants at the top of the file for these URLS?
https://codereview.adblockplus.org/29712664/diff/29712665/background.js File background.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newco... background.js:71: additionalSubscriptions: false, Detail: Please also mention this new parameter in the README. https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newco... background.js:71: additionalSubscriptions: false, Why not implement this the same way as "blockedURLs" and change the value to a string that contains a list of URLs? https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newco... background.js:112: additional_subscriptions: (params.additionalSubscriptions) ? ["https://easylist-downloads.adblockplus.org/easylistgermany+easylist.txt"] : [], On 2018/03/02 09:57:01, kzar wrote: > Nit: The paranthesis around params.additionalSubscriptions seem pointless. Sounds like a personal preference which I usually leave up to the implementer. https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.... js/desktop-options.js:243: else if (this == collections.filterLists) On 2018/03/01 21:52:34, saroyanm wrote: > I wonder if I also should consider the case of removing checkbox from the > Privacy & Security section for the Block additional tracking and Block social > media icons tracking, when matched ? > > Should we still should allow changing or removing the language subscriptions > from the General tab ? You're right that we should also consider other places in the UI where the user can remove filter lists. But I'd say let's base such changes on the spec. I've created https://gitlab.com/eyeo/specs/spec/issues/154 to update the spec accordinly. For now let's tackle anything that's easy to fix and that doesn't require any large changes so that we can at least reestablish the previous behavior. https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.... js/desktop-options.js:243: else if (this == collections.filterLists) This if-statement is redundant because in the CSS selector you're already making sure that only `#all-filter-lists-table` is affected by this. There's no harm in marking filter lists as "preconfigured" irregardless of the table they're in. https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.... js/desktop-options.js:245: // List preinstalled by administrators Detail: This is already documented under https://hg.adblockplus.org/adblockpluschrome/file/ffd89bbfcd6f/lib/prefs.js#l194 so let's not duplicate that here. https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.... js/desktop-options.js:249: element.classList.add("non-removable"); Detail: What about keeping the name more objective (e.g. "preconfigured")? That way we could easily reuse it in the future without having to add more classes.
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#newco... background.js:71: additionalSubscriptions: false, On 2018/03/02 11:33:35, Thomas Greiner wrote: > Detail: Please also mention this new parameter in the README. Done. https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newco... background.js:71: additionalSubscriptions: false, On 2018/03/02 11:33:35, Thomas Greiner wrote: > Why not implement this the same way as "blockedURLs" and change the value to a > string that contains a list of URLs? Done. https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newco... background.js:112: additional_subscriptions: (params.additionalSubscriptions) ? ["https://easylist-downloads.adblockplus.org/easylistgermany+easylist.txt"] : [], On 2018/03/02 09:57:01, kzar wrote: > Nit: The paranthesis around params.additionalSubscriptions seem pointless. > > Nit: This line is way too long, it passes linting since it contains a URL but > the purpose of that exception is to allow long URLs which would have to be split > up otherwise. How about having a few constants at the top of the file for these > URLS? Done. https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.... js/desktop-options.js:243: else if (this == collections.filterLists) On 2018/03/02 11:33:36, Thomas Greiner wrote: > On 2018/03/01 21:52:34, saroyanm wrote: > > I wonder if I also should consider the case of removing checkbox from the > > Privacy & Security section for the Block additional tracking and Block social > > media icons tracking, when matched ? > > > > Should we still should allow changing or removing the language subscriptions > > from the General tab ? > > You're right that we should also consider other places in the UI where the user > can remove filter lists. But I'd say let's base such changes on the spec. I've > created https://gitlab.com/eyeo/specs/spec/issues/154 to update the spec > accordinly. > > For now let's tackle anything that's easy to fix and that doesn't require any > large changes so that we can at least reestablish the previous behavior. Done. https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.... js/desktop-options.js:243: else if (this == collections.filterLists) On 2018/03/02 11:33:35, Thomas Greiner wrote: > This if-statement is redundant because in the CSS selector you're already making > sure that only `#all-filter-lists-table` is affected by this. > > There's no harm in marking filter lists as "preconfigured" irregardless of the > table they're in. Done. I moved implementation out of the for loop, as we need to make "preconfigured" element, rather than each checkbox. https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.... js/desktop-options.js:245: // List preinstalled by administrators On 2018/03/02 11:33:35, Thomas Greiner wrote: > Detail: This is already documented under > https://hg.adblockplus.org/adblockpluschrome/file/ffd89bbfcd6f/lib/prefs.js#l194 > so let's not duplicate that here. Done. https://codereview.adblockplus.org/29712664/diff/29712665/js/desktop-options.... js/desktop-options.js:249: element.classList.add("non-removable"); On 2018/03/02 11:33:35, Thomas Greiner wrote: > Detail: What about keeping the name more objective (e.g. "preconfigured")? That > way we could easily reuse it in the future without having to add more classes. Done.
On 2018/03/01 22:14:21, a.giammarchi wrote:
> On 2018/03/01 21:52:34, saroyanm wrote:
> > @Andrea: Is there an easy way to disable the eslisting during development,
> > without modifying package.json ?
>
> not sure what you need to disable it for, but one little hack I use from time
to
> time is to `eval("debugger;")` which AFAIK is allowed by our rules.
>
> Would that help?
Throws error for me.. both debugger; and eval("debugger;")
Error msg: "error eval can be harmful"
https://codereview.adblockplus.org/29712664/diff/29713577/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29712664/diff/29713577/skin/desktop-option... skin/desktop-options.css:1080: display: none !important; I had to use import because of https://hg.adblockplus.org/adblockplusui/file/tip/skin/desktop-options.css#l948 style
Addressed Jeen's suggetion in the ticket, about disabled checkbox.
https://codereview.adblockplus.org/29712664/diff/29712665/background.js File background.js (right): https://codereview.adblockplus.org/29712664/diff/29712665/background.js#newco... background.js:107: notifications_ignoredcategories: (params.showNotificationUI) ? ["*"] : [], Detail: This change is unrelated to this issue and doesn't seem to add any value. https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.htm... desktop-options.html:89: <button data-preconfigured="hide" data-action="toggle-remove-subscription" role="checkbox" class="control icon"></button> As I mentioned, let's focus on fixing this regression and not make any further changes at this point.
https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.htm... 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, Thomas Greiner wrote: > As I mentioned, let's focus on fixing this regression and not make any further > changes at this point. I'm not sure why this comment is located here. The only non regression change I think is the introduction of the grey Icon and small changes in the background page, but I assume you are referring to some other change here ?
(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#newco... background.js:112: additional_subscriptions: (params.additionalSubscriptions) ? ["https://easylist-downloads.adblockplus.org/easylistgermany+easylist.txt"] : [], On 2018/03/02 13:50:29, saroyanm wrote: > On 2018/03/02 09:57:01, kzar wrote: > > Nit: The paranthesis around params.additionalSubscriptions seem pointless. > > > > Nit: This line is way too long, it passes linting since it contains a URL but > > the purpose of that exception is to allow long URLs which would have to be > split > > up otherwise. How about having a few constants at the top of the file for > these > > URLS? > > Done. Thanks
https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.htm... desktop-options.html:89: <button data-preconfigured="hide" data-action="toggle-remove-subscription" role="checkbox" class="control icon"></button> What about reversing that and instead write `data-hide="preconfigured"`? That way we could reuse the attribute for hiding elements using other conditions in the future (e.g. `data-hide="weekend"` to hide elements only on weekends). https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.htm... desktop-options.html:90: <button data-preconfigured="show" role="checkbox" class="control icon" disabled></button> This button is redundant because we could simply set the existing button to disabled when we initialize this collection. The browser should then no longer trigger any of its actions. That way we'd no longer need to - keep this button in sync with the other one - introduce `data-preconfigured="show"` - use `display: none !important` to hide one of the two buttons I'd recommend also setting `cursor: default` on disabled elements for a more consistent UX. https://codereview.adblockplus.org/29712664/diff/29713622/skin/icons/checkbox... File skin/icons/checkbox.svg (right): https://codereview.adblockplus.org/29712664/diff/29713622/skin/icons/checkbox... skin/icons/checkbox.svg:13: <rect width="17" height="17" fill="#b3b3b3" stroke="#b3b3b3" rx="2" x=".5" y=".5" /> Detail: This color is not listed in the options page style guide (see https://gitlab.com/eyeo/specs/spec/blob/master/spec/abp/options-page-style-gu...).
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.htm... desktop-options.html:89: <button data-preconfigured="hide" data-action="toggle-remove-subscription" role="checkbox" class="control icon"></button> On 2018/03/05 14:52:10, Thomas Greiner wrote: > What about reversing that and instead write `data-hide="preconfigured"`? That > way we could reuse the attribute for hiding elements using other conditions in > the future (e.g. `data-hide="weekend"` to hide elements only on weekends). Done. https://codereview.adblockplus.org/29712664/diff/29713622/desktop-options.htm... desktop-options.html:90: <button data-preconfigured="show" role="checkbox" class="control icon" disabled></button> On 2018/03/05 14:52:10, Thomas Greiner wrote: > This button is redundant because we could simply set the existing button to > disabled when we initialize this collection. The browser should then no longer > trigger any of its actions. > > That way we'd no longer need to > - keep this button in sync with the other one > - introduce `data-preconfigured="show"` > - use `display: none !important` to hide one of the two buttons > > I'd recommend also setting `cursor: default` on disabled elements for a more > consistent UX. Done. https://codereview.adblockplus.org/29712664/diff/29713622/skin/icons/checkbox... File skin/icons/checkbox.svg (right): https://codereview.adblockplus.org/29712664/diff/29713622/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/05 14:52:10, Thomas Greiner wrote: > Detail: This color is not listed in the options page style guide (see > https://gitlab.com/eyeo/specs/spec/blob/master/spec/abp/options-page-style-gu...). I changed the color to match Acceptable Ads and created this -> https://gitlab.com/eyeo/specs/spec/issues/155
https://codereview.adblockplus.org/29712664/diff/29713622/skin/icons/checkbox... File skin/icons/checkbox.svg (right): https://codereview.adblockplus.org/29712664/diff/29713622/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/06 15:09:53, saroyanm wrote: > On 2018/03/05 14:52:10, Thomas Greiner wrote: > > Detail: This color is not listed in the options page style guide (see > > > https://gitlab.com/eyeo/specs/spec/blob/master/spec/abp/options-page-style-gu...). > > I changed the color to match Acceptable Ads and created this -> > https://gitlab.com/eyeo/specs/spec/issues/155 Great, since Jeen already responded there, we could already apply here suggested color changes here. 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/desktop-options.js:244: getPref("additional_subscriptions", (additionalSubscriptions) => I just noticed that this is called each time we update an item in a collection. So instead of requesting this preference each time an item is updated, let's intead request it once on page load as we do with `acceptableAdsUrl` and other preferences. https://codereview.adblockplus.org/29712664/diff/29715733/js/desktop-options.... js/desktop-options.js:249: if (this == collections.protection) We tend to avoid hard-coding behavior for specific lists in `Collection` so here's one way to avoid that: let affectedElements = element.querySelectorAll("[data-disable~='preconfigured']"); for (let affectedElement of affectedElements) { affectedElement.disabled = true; } We could even reuse that approach for the code above, for example, by handling `[data-disable~="acceptable-ads"]` to avoid hardcoding the list there. But that may be out of scope for this issue so just wanted to mention it.
Thanks Thomas, this is ready for another review round. https://codereview.adblockplus.org/29712664/diff/29713622/skin/icons/checkbox... File skin/icons/checkbox.svg (right): https://codereview.adblockplus.org/29712664/diff/29713622/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/07 17:45:38, Thomas Greiner wrote: > On 2018/03/06 15:09:53, saroyanm wrote: > > On 2018/03/05 14:52:10, Thomas Greiner wrote: > > > Detail: This color is not listed in the options page style guide (see > > > > > > https://gitlab.com/eyeo/specs/spec/blob/master/spec/abp/options-page-style-gu...). > > > > I changed the color to match Acceptable Ads and created this -> > > https://gitlab.com/eyeo/specs/spec/issues/155 > > Great, since Jeen already responded there, we could already apply here suggested > color changes here. Done. 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/desktop-options.js:244: getPref("additional_subscriptions", (additionalSubscriptions) => On 2018/03/07 17:45:38, Thomas Greiner wrote: > I just noticed that this is called each time we update an item in a collection. > So instead of requesting this preference each time an item is updated, let's > intead request it once on page load as we do with `acceptableAdsUrl` and other > preferences. Done, we still need to refactor code to avoid the callback hells, there is an implementation in Patchset #7 which fixes that by using promises, but as we discussed, we would like to fix that separately, in order to introduce as few code as possible in current review. I think we can refactor the callback hell as part of -> https://issues.adblockplus.org/ticket/5747 https://codereview.adblockplus.org/29712664/diff/29715733/js/desktop-options.... js/desktop-options.js:249: if (this == collections.protection) On 2018/03/07 17:45:38, Thomas Greiner wrote: > We tend to avoid hard-coding behavior for specific lists in `Collection` so > here's one way to avoid that: > > let affectedElements = > element.querySelectorAll("[data-disable~='preconfigured']"); > for (let affectedElement of affectedElements) > { > affectedElement.disabled = true; > } > > We could even reuse that approach for the code above, for example, by handling > `[data-disable~="acceptable-ads"]` to avoid hardcoding the list there. But that > may be out of scope for this issue so just wanted to mention it. Done.
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/desktop-options.js:244: getPref("additional_subscriptions", (additionalSubscriptions) => On 2018/03/08 18:03:36, saroyanm wrote: > Done, we still need to refactor code to avoid the callback hells, there is an > implementation in Patchset #7 which fixes that by using promises, but as we > discussed, we would like to fix that separately, in order to introduce as few > code as possible in current review. I think we can refactor the callback hell as > part of -> https://issues.adblockplus.org/ticket/5747 I agree that this can be tackled separately as part of #5747. 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; Detail: Isn't this redundant since we're setting `fill="#b3b3b3"` in the SVG anyway? 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" /> 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.
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/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. 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/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.
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.
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. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
