Issue 29587717: Issue 5926 - Fix missing string errors (Closed)
DescriptionThis is a regression introduced by -> https://codereview.adblockplus.org/29572786/
Patch Set 1 : #
Total comments: 4
MessagesTotal messages: 5
@Ire can you please have a look.
LGTM. One NIT/Suggestion which you're free to take or ignore :) https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js#... desktop-options.js:511: if (subscription.recommended != "ads") NIT/Suggestion: I think just checking that the type is no ads may be encompass more than we want (in the future). Assuming that a subscription could be added with a different type than ads, that doesn't necessarily mean it should be part of this list. I think it would be clearer to check against a list of the recommended names/types e.g. let recommendedSubscriptions = ["EasyPrivacy", "Fanboy's Social Blocking List"] if (recommendedSubscriptions.includes(subscription.originalTitle))
https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js#... desktop-options.js:511: if (subscription.recommended != "ads") On 2017/10/25 09:12:52, ire wrote: > NIT/Suggestion: I think just checking that the type is no ads may be encompass > more than we want (in the future). Assuming that a subscription could be added > with a different type than ads, that doesn't necessarily mean it should be part > of this list. I think it would be clearer to check against a list of the > recommended names/types e.g. > > let recommendedSubscriptions = ["EasyPrivacy", "Fanboy's Social Blocking List"] > if (recommendedSubscriptions.includes(subscription.originalTitle)) Thanks for bringing this up, yes and no: Currently we do recommend(In the subscriptions.xml) Languages filter lists which are of type "ads" and two other, which in this case "EasyPrivacy", "Fanboy's Social Blocking List", so with current logic whatever is not of type "ads" goes to the "PRIVACY & SECURITY" section, that was initial intention of current implementation if there were more we might also consider changing the section title, but this was changed and in future we will also have custom titles for some other special filter lists like ("Adblock warning removal list, Spam and etc.") which are shown in the More Filters section, but introduction of that logic will require change in the current implementation. The current implementation that you see here I tried to make it consistent with the current logic we have, see for example -> https://hg.adblockplus.org/adblockplusui/file/tip/desktop-options.js#l388 So current logic says, what is not of type ads and recommended, treat differently - Move to the Privacy & Secutiry section (ex.: Recommendation section) and add custom titles to it. In order to addd custom title to other filter lists we will need to add them to the subscrioptions.xml as well, but in that case this logic will be changed to the one you have mentioned, or we might introduce additional attribute which will differentiate the "Privacy" related subscriptions.
https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js#... desktop-options.js:511: if (subscription.recommended != "ads") On 2017/10/25 13:42:32, saroyanm wrote: > On 2017/10/25 09:12:52, ire wrote: > > NIT/Suggestion: I think just checking that the type is no ads may be encompass > > more than we want (in the future). Assuming that a subscription could be added > > with a different type than ads, that doesn't necessarily mean it should be > part > > of this list. I think it would be clearer to check against a list of the > > recommended names/types e.g. > > > > let recommendedSubscriptions = ["EasyPrivacy", "Fanboy's Social Blocking > List"] > > if (recommendedSubscriptions.includes(subscription.originalTitle)) > > Thanks for bringing this up, yes and no: Currently we do recommend(In the > subscriptions.xml) Languages filter lists which are of type "ads" and two other, > which in this case "EasyPrivacy", "Fanboy's Social Blocking List", so with > current logic whatever is not of type "ads" goes to the "PRIVACY & SECURITY" > section, that was initial intention of current implementation if there were more > we might also consider changing the section title, but this was changed and in > future we will also have custom titles for some other special filter lists like > ("Adblock warning removal list, Spam and etc.") which are shown in the More > Filters section, but introduction of that logic will require change in the > current implementation. > > The current implementation that you see here I tried to make it consistent with > the current logic we have, see for example -> > https://hg.adblockplus.org/adblockplusui/file/tip/desktop-options.js#l388 > > So current logic says, what is not of type ads and recommended, treat > differently - Move to the Privacy & Secutiry section (ex.: Recommendation > section) and add custom titles to it. > > In order to addd custom title to other filter lists we will need to add them to > the subscrioptions.xml as well, but in that case this logic will be changed to > the one you have mentioned, or we might introduce additional attribute which > will differentiate the "Privacy" related subscriptions. Here is also the relevant discussion where we decided to implement like that -> https://codereview.adblockplus.org/29478597/diff/29517651/new-options.js#newc... But it's longer :/
Thanks! LGTM again :) https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29587717/diff/29587720/desktop-options.js#... desktop-options.js:511: if (subscription.recommended != "ads") On 2017/10/25 13:42:32, saroyanm wrote: > On 2017/10/25 09:12:52, ire wrote: > > NIT/Suggestion: I think just checking that the type is no ads may be encompass > > more than we want (in the future). Assuming that a subscription could be added > > with a different type than ads, that doesn't necessarily mean it should be > part > > of this list. I think it would be clearer to check against a list of the > > recommended names/types e.g. > > > > let recommendedSubscriptions = ["EasyPrivacy", "Fanboy's Social Blocking > List"] > > if (recommendedSubscriptions.includes(subscription.originalTitle)) > > Thanks for bringing this up, yes and no: Currently we do recommend(In the > subscriptions.xml) Languages filter lists which are of type "ads" and two other, > which in this case "EasyPrivacy", "Fanboy's Social Blocking List", so with > current logic whatever is not of type "ads" goes to the "PRIVACY & SECURITY" > section, that was initial intention of current implementation if there were more > we might also consider changing the section title, but this was changed and in > future we will also have custom titles for some other special filter lists like > ("Adblock warning removal list, Spam and etc.") which are shown in the More > Filters section, but introduction of that logic will require change in the > current implementation. > > The current implementation that you see here I tried to make it consistent with > the current logic we have, see for example -> > https://hg.adblockplus.org/adblockplusui/file/tip/desktop-options.js#l388 > > So current logic says, what is not of type ads and recommended, treat > differently - Move to the Privacy & Secutiry section (ex.: Recommendation > section) and add custom titles to it. > > In order to addd custom title to other filter lists we will need to add them to > the subscrioptions.xml as well, but in that case this logic will be changed to > the one you have mentioned, or we might introduce additional attribute which > will differentiate the "Privacy" related subscriptions. Thanks very much for the explanation. I understand now and your implementation looks like the most suitable for this case. If/when we decide to change the rule of "whatever is not of type "ads" goes to the "PRIVACY & SECURITY" section", then we can update this as well. 
|
|||||||||||||||||||||||||||||||||||||||
