Issue 29609587: Issue 6031 - Implement Acceptable Ads notification (Closed)
|
Created:
Nov. 15, 2017, 5:01 p.m. by saroyanm Modified:
Nov. 22, 2017, 10:44 a.m. Reviewers:
Thomas Greiner CC:
ire Visibility:
Public. |
DescriptionIssue 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 : #
MessagesTotal messages: 10
@Thomas while Ire will be on vacation, can you please have a look, considering the fact that we already done this -> https://codereview.adblockplus.org/29519669/ should be relatively easy.
https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.htm... desktop-options.html:100: <div id="tracking-notification"> Suggestion: The naming is inconsistent. Here are the names I'm referring to: - i18n_options_tracking_notification_1 - hide-tracking-notification - show-notification - showTrackingWarning - tracking-notification - ui_warn_tracking As you notice, in some cases we call it "notification" while in others "warning". The most difficult to change is the preference name due to #6042 so I'd recommend changing the names from "notification" to "warning". https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.js#... desktop-options.js:611: E("acceptable-ads").classList.remove("show-notification"); Let's remove this action and instead react to changes to "ui_warn_tracking" (similar to the way we react to subscription changes in `onSubscriptionMessage()`) so that we keep the UI in sync with the state of the extension. Also, I don't see "ui_warn_tracking" being handled in `onPrefMessage()` even though you're listening to its changes. https://codereview.adblockplus.org/29609587/diff/29609588/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29609587/diff/29609588/locale/en_US/deskto... locale/en_US/desktop-options.json:80: "message": "If you prefer extra privacy, select the '<strong>Only allow Acceptable Ads that are privacy-friendly</strong>' checkbox below." "Only allow Acceptable Ads that are privacy-friendly" has been renamed. Generally, I noticed that we mention a lot of filter list and option names in those strings. So we could use placeholders for those in translatable strings which would allow us to ensure that the texts for those are always exactly the same. https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:623: #acceptable-ads.show-notification #tracking-notification Suggestion: You could avoid setting "display" twice and you wouldn't need to care about its value if you use `:not()`. i.e. #acceptable-ads:not(.show-notification) #tracking-notification { display: none; } https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:635: background-color: #fefbe3; Detail: These colors aren't mentioned in the style guide so we should add them to the spec. https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:641: right: 1rem; What about right-to-left environments? https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:645: #tracking-notification .link Detail: Links also use `cursor: pointer`. Alternatively, we could use a `<a>` element instead of a `<button>` here. https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:939: Used for translatable screen reader only conten. Typo: Replace "conten" with "content". https://codereview.adblockplus.org/29609587/diff/29609588/skin/icons/delete.svg File skin/icons/delete.svg (right): https://codereview.adblockplus.org/29609587/diff/29609588/skin/icons/delete.s... skin/icons/delete.svg:18: <polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71 5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29 25.41 24 42.71 6.71" transform="translate(0, 240)" fill="#9A9A49"/> Detail: This color isn't mentioned anywhere in the style guide.
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.htm... desktop-options.html:100: <div id="tracking-notification"> On 2017/11/16 19:25:05, Thomas Greiner wrote: > Suggestion: The naming is inconsistent. Here are the names I'm referring to: > - i18n_options_tracking_notification_1 > - hide-tracking-notification > - show-notification > - showTrackingWarning > - tracking-notification > - ui_warn_tracking > > As you notice, in some cases we call it "notification" while in others > "warning". The most difficult to change is the preference name due to #6042 so > I'd recommend changing the names from "notification" to "warning". Agree, done. https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29609587/diff/29609588/desktop-options.js#... desktop-options.js:611: E("acceptable-ads").classList.remove("show-notification"); On 2017/11/16 19:25:05, Thomas Greiner wrote: > Let's remove this action and instead react to changes to "ui_warn_tracking" > (similar to the way we react to subscription changes in > `onSubscriptionMessage()`) so that we keep the UI in sync with the state of the > extension. > > Also, I don't see "ui_warn_tracking" being handled in `onPrefMessage()` even > though you're listening to its changes. I agree, done. https://codereview.adblockplus.org/29609587/diff/29609588/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29609587/diff/29609588/locale/en_US/deskto... locale/en_US/desktop-options.json:80: "message": "If you prefer extra privacy, select the '<strong>Only allow Acceptable Ads that are privacy-friendly</strong>' checkbox below." On 2017/11/16 19:25:05, Thomas Greiner wrote: > "Only allow Acceptable Ads that are privacy-friendly" has been renamed. > > Generally, I noticed that we mention a lot of filter list and option names in > those strings. So we could use placeholders for those in translatable strings > which would allow us to ensure that the texts for those are always exactly the > same. Done. https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:623: #acceptable-ads.show-notification #tracking-notification On 2017/11/16 19:25:06, Thomas Greiner wrote: > Suggestion: You could avoid setting "display" twice and you wouldn't need to > care about its value if you use `:not()`. > > i.e. > #acceptable-ads:not(.show-notification) #tracking-notification > { > display: none; > } Right, done. https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:635: background-color: #fefbe3; On 2017/11/16 19:25:05, Thomas Greiner wrote: > Detail: These colors aren't mentioned in the style guide so we should add them > to the spec. Yes in general style guide for this notification is missing from the pull request. -> https://bitbucket.org/adblockplus/spec/pull-requests/81/issue-87-new-proposal... https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:641: right: 1rem; On 2017/11/16 19:25:06, Thomas Greiner wrote: > What about right-to-left environments? Done. https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:645: #tracking-notification .link On 2017/11/16 19:25:06, Thomas Greiner wrote: > Detail: Links also use `cursor: pointer`. > > Alternatively, we could use a `<a>` element instead of a `<button>` here. `cursor: pointer` is inherited from the class `button`. I don't think that we should use `<a>` here, as semantically this is a button which looks like a link. Similar to the `About Adblock Plus` button in the sidebar, while for example `Contribute` which looks like a button, is actually a link, I tried to be here consistent to that logic. https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:939: Used for translatable screen reader only conten. On 2017/11/16 19:25:06, Thomas Greiner wrote: > Typo: Replace "conten" with "content". Done. https://codereview.adblockplus.org/29609587/diff/29609588/skin/icons/delete.svg File skin/icons/delete.svg (right): https://codereview.adblockplus.org/29609587/diff/29609588/skin/icons/delete.s... skin/icons/delete.svg:18: <polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71 5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29 25.41 24 42.71 6.71" transform="translate(0, 240)" fill="#9A9A49"/> On 2017/11/16 19:25:06, Thomas Greiner wrote: > Detail: This color isn't mentioned anywhere in the style guide. Yes, I asked to provide a style guide, for pull request -> https://bitbucket.org/adblockplus/spec/pull-requests/81/issue-87-new-proposal... I'll followup on the button colors if they will not exist still. 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#... desktop-options.js:31: let {setElementText} = ext.i18n; Didin't we convert all ext. to browser. ? I'm not sure if this is a correct use. https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#... desktop-options.js:911: setElementText(E("tracking-warning-1"), "options_tracking_warning_1", Ideally I think we should be able to specify "translatable" arguments somehow in the HTML data ? Maybe <p data-i18n="options_tracking_warning_1" data-args="common_feature_privacy_title,options_acceptableAds_ads_label"></p> If you agree, I'll create another ticket to handle that, or can handle in this ticket as well, depending how urgent are this changes.
https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... locale/en_US/desktop-options.json:72: "message": "We noticed you have both <strong>'$easy-privacy$'</strong> and '<strong>$acceptable-ads$</strong>' enabled.", Detail: <strong> should go inside of "'" for consistency
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#... desktop-options.js:1379: if (!value) Does it make sense to check against value == false ? In case the preferences gets corrupted, or doesn't load ?
https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:635: background-color: #fefbe3; On 2017/11/17 16:10:39, saroyanm wrote: > On 2017/11/16 19:25:05, Thomas Greiner wrote: > > Detail: These colors aren't mentioned in the style guide so we should add them > > to the spec. > > Yes in general style guide for this notification is missing from the pull > request. -> > https://bitbucket.org/adblockplus/spec/pull-requests/81/issue-87-new-proposal... Acknowledged. https://codereview.adblockplus.org/29609587/diff/29609588/skin/desktop-option... skin/desktop-options.css:645: #tracking-notification .link On 2017/11/17 16:10:39, saroyanm wrote: > `cursor: pointer` is inherited from the class `button`. Indeed, I missed that. https://codereview.adblockplus.org/29609587/diff/29609588/skin/icons/delete.svg File skin/icons/delete.svg (right): https://codereview.adblockplus.org/29609587/diff/29609588/skin/icons/delete.s... skin/icons/delete.svg:18: <polygon points="42.71 6.71 41.29 5.29 24 22.59 6.71 5.29 5.29 6.71 22.59 24 5.29 41.29 6.71 42.71 24 25.41 41.29 42.71 42.71 41.29 25.41 24 42.71 6.71" transform="translate(0, 240)" fill="#9A9A49"/> On 2017/11/17 16:10:39, saroyanm wrote: > On 2017/11/16 19:25:06, Thomas Greiner wrote: > > Detail: This color isn't mentioned anywhere in the style guide. > > Yes, I asked to provide a style guide, for pull request -> > https://bitbucket.org/adblockplus/spec/pull-requests/81/issue-87-new-proposal... > I'll followup on the button colors if they will not exist still. Acknowledged. https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.htm... desktop-options.html:106: <p id="tracking-warning-3" class="i18n_options_tracking_warning_3"></p> Detail: The "i18n_*" class here is redundant because you're overwriting it in desktop-options.js anyway. 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#... desktop-options.js:31: let {setElementText} = ext.i18n; On 2017/11/17 16:10:39, saroyanm wrote: > Didin't we convert all ext. to browser. ? > I'm not sure if this is a correct use. No, we merely got rid of the abstraction layer for Web Extension APIs. This method, however, is not part of that. Alternatively, you could call it as `ext.i18n.setElementText()` if you don't like doing this import. https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#... desktop-options.js:911: setElementText(E("tracking-warning-1"), "options_tracking_warning_1", On 2017/11/17 16:10:39, saroyanm wrote: > Ideally I think we should be able to specify "translatable" arguments somehow in > the HTML data ? > > Maybe <p data-i18n="options_tracking_warning_1" > data-args="common_feature_privacy_title,options_acceptableAds_ads_label"></p> > > If you agree, I'll create another ticket to handle that, or can handle in this > ticket as well, depending how urgent are this changes. Let's wait for further cases where we'd need that. Because at this point it wouldn't add any value since this is the only part in the code where we use it. https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#... desktop-options.js:1379: if (!value) On 2017/11/17 16:45:14, saroyanm wrote: > Does it make sense to check against value == false ? > In case the preferences gets corrupted, or doesn't load ? I'd assume not. If we cannot load the setting, we should get its default value as defined in lib/prefs.js. It would help in case we accidentally set the preference value to `undefined`, `null` or `""` but then we'd need to use `value === false` since `"" == false` and `[] == false` both evaluate to `true`. https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#... desktop-options.js:1380: E("acceptable-ads").classList.remove("show-warning"); What if the value changes to `true`? We'd probably want to reuse `hasPrivacyConflict()` here again in that case. While this is currently not possible via the UI, it could be useful for testing and it could help avoid issues with this in the future. https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... locale/en_US/desktop-options.json:72: "message": "We noticed you have both <strong>'$easy-privacy$'</strong> and '<strong>$acceptable-ads$</strong>' enabled.", On 2017/11/17 16:13:26, saroyanm wrote: > Detail: <strong> should go inside of "'" for consistency Might be a personal preference but I agree. In any case, we should sync up with our technical writers to keep this consistent also in the future. https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... locale/en_US/desktop-options.json:74: "easy-privacy": { Detail: I'd recommend avoiding special characters such as "-" because it's not clear which characters we are allowed to use in the placeholder IDs according to https://developer.chrome.com/apps/i18n-messages#placeholders https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... locale/en_US/desktop-options.json:74: "easy-privacy": { Detail: The filter list behind the "privacy" feature might change so, while we can mention "EasyPrivacy" as an example, we should use a more generic name for the placeholder ID. https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... locale/en_US/desktop-options.json:75: "content": "$1" Detail: Let's also include the "example" property for each placeholder as we do for others in this file.
Sorry that it took longer to address the comments. New patch uploaded. Ready for the review. https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.htm... desktop-options.html:106: <p id="tracking-warning-3" class="i18n_options_tracking_warning_3"></p> On 2017/11/20 18:36:04, Thomas Greiner wrote: > Detail: The "i18n_*" class here is redundant because you're overwriting it in > desktop-options.js anyway. Done. 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#... desktop-options.js:31: let {setElementText} = ext.i18n; On 2017/11/20 18:36:05, Thomas Greiner wrote: > On 2017/11/17 16:10:39, saroyanm wrote: > > Didin't we convert all ext. to browser. ? > > I'm not sure if this is a correct use. > > No, we merely got rid of the abstraction layer for Web Extension APIs. This > method, however, is not part of that. > > Alternatively, you could call it as `ext.i18n.setElementText()` if you don't > like doing this import. I like, import here, as it's somehow consistent with other "{getMessage}" import. https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#... desktop-options.js:911: setElementText(E("tracking-warning-1"), "options_tracking_warning_1", On 2017/11/20 18:36:05, Thomas Greiner wrote: > On 2017/11/17 16:10:39, saroyanm wrote: > > Ideally I think we should be able to specify "translatable" arguments somehow > in > > the HTML data ? > > > > Maybe <p data-i18n="options_tracking_warning_1" > > data-args="common_feature_privacy_title,options_acceptableAds_ads_label"></p> > > > > If you agree, I'll create another ticket to handle that, or can handle in this > > ticket as well, depending how urgent are this changes. > > Let's wait for further cases where we'd need that. Because at this point it > wouldn't add any value since this is the only part in the code where we use it. Agree. https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#... desktop-options.js:1379: if (!value) On 2017/11/20 18:36:05, Thomas Greiner wrote: > On 2017/11/17 16:45:14, saroyanm wrote: > > Does it make sense to check against value == false ? > > In case the preferences gets corrupted, or doesn't load ? > > I'd assume not. If we cannot load the setting, we should get its default value > as defined in lib/prefs.js. > > It would help in case we accidentally set the preference value to `undefined`, > `null` or `""` but then we'd need to use `value === false` since `"" == false` > and `[] == false` both evaluate to `true`. I see an issue with strict equation(not sure if that might be case), when the Browser passes non Boolean value (ex. String) ? I think we can leave it as it is right now. https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#... desktop-options.js:1380: E("acceptable-ads").classList.remove("show-warning"); On 2017/11/20 18:36:05, Thomas Greiner wrote: > What if the value changes to `true`? We'd probably want to reuse > `hasPrivacyConflict()` here again in that case. > > While this is currently not possible via the UI, it could be useful for testing > and it could help avoid issues with this in the future. Agree. Done. https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... locale/en_US/desktop-options.json:72: "message": "We noticed you have both <strong>'$easy-privacy$'</strong> and '<strong>$acceptable-ads$</strong>' enabled.", On 2017/11/20 18:36:05, Thomas Greiner wrote: > On 2017/11/17 16:13:26, saroyanm wrote: > > Detail: <strong> should go inside of "'" for consistency > > Might be a personal preference but I agree. In any case, we should sync up with > our technical writers to keep this consistent also in the future. I agree that this should be documented somewhere, it was more note for myself, to make it consistent, as I moved the tag outside while I was testing the passed variable. https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... locale/en_US/desktop-options.json:74: "easy-privacy": { On 2017/11/20 18:36:05, Thomas Greiner wrote: > Detail: I'd recommend avoiding special characters such as "-" because it's not > clear which characters we are allowed to use in the placeholder IDs according to > https://developer.chrome.com/apps/i18n-messages#placeholders Agree, done. https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... locale/en_US/desktop-options.json:74: "easy-privacy": { On 2017/11/20 18:36:05, Thomas Greiner wrote: > Detail: The filter list behind the "privacy" feature might change so, while we > can mention "EasyPrivacy" as an example, we should use a more generic name for > the placeholder ID. Okey, I use "tracking" now. https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... locale/en_US/desktop-options.json:75: "content": "$1" On 2017/11/20 18:36:05, Thomas Greiner wrote: > Detail: Let's also include the "example" property for each placeholder as we do > for others in this file. Done.
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#... desktop-options.js:1379: if (!value) On 2017/11/21 14:58:32, saroyanm wrote: > I see an issue with strict equation(not sure if that might be case), when the > Browser passes non Boolean value (ex. String) ? > > I think we can leave it as it is right now. I don't understand what you mean but I'm fine with relying on truthy and falsy values for now since we do the same for "notifications_showui" above. 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#... desktop-options.js:1380: E("acceptable-ads").classList.add("show-warning"); As hinted at in https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#..., we should only show the warning if we actually detect a conflict. e.g. let showWarning = (value && hasPrivacyConflict()); E("acceptable-ads").classList.toggle("show-warning", showWarning); https://codereview.adblockplus.org/29609587/diff/29613759/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29609587/diff/29613759/locale/en_US/deskto... locale/en_US/desktop-options.json:76: "example": "EasyPrivacy" Detail: These examples don't reflect the feature label (i.e. "Block additional tracking" and "Allow Acceptable Ads") as seen in the spec but the filter list title and are therefore not representative of the values we'd expect for those placeholders. https://codereview.adblockplus.org/29609587/diff/29613759/locale/en_US/deskto... locale/en_US/desktop-options.json:92: "privacy-friendly-acceptable-ads": { Detail: See https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... and https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto...
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#... desktop-options.js:1380: E("acceptable-ads").classList.add("show-warning"); On 2017/11/21 17:17:29, Thomas Greiner wrote: > As hinted at in > https://codereview.adblockplus.org/29609587/diff/29611601/desktop-options.js#..., > we should only show the warning if we actually detect a conflict. > > e.g. > let showWarning = (value && hasPrivacyConflict()); > E("acceptable-ads").classList.toggle("show-warning", showWarning); Right, thanks, done. https://codereview.adblockplus.org/29609587/diff/29613759/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29609587/diff/29613759/locale/en_US/deskto... locale/en_US/desktop-options.json:76: "example": "EasyPrivacy" On 2017/11/21 17:17:29, Thomas Greiner wrote: > Detail: These examples don't reflect the feature label (i.e. "Block additional > tracking" and "Allow Acceptable Ads") as seen in the spec but the filter list > title and are therefore not representative of the values we'd expect for those > placeholders. Done. https://codereview.adblockplus.org/29609587/diff/29613759/locale/en_US/deskto... locale/en_US/desktop-options.json:92: "privacy-friendly-acceptable-ads": { On 2017/11/21 17:17:29, Thomas Greiner wrote: > Detail: See > https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... > and > https://codereview.adblockplus.org/29609587/diff/29611601/locale/en_US/deskto... Sorry, done.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
