Issue 29565858: Issue 5803 - add privacy friendly acceptable ads preference (Closed)
DescriptionIssue 5803 - add privacy friendly acceptable ads preference
Patch Set 1 #
Total comments: 1
Patch Set 2 : Removed the link and changed wording #
Total comments: 3
Patch Set 3 : Use consistent string name #MessagesTotal messages: 7
https://codereview.adblockplus.org/29565858/diff/29565859/pages/preferences.tmpl File pages/preferences.tmpl (right): https://codereview.adblockplus.org/29565858/diff/29565859/pages/preferences.t... pages/preferences.tmpl:228: {{"Location of the filter list used to\n<a href=\"acceptable-ads\">allow privacy-friendly non-intrusive ads</a>."|translate("subscriptions_exceptionsurl-privacy")}} @Wspee: DO we have, or planing to have a dedicated page for describing Privacy Friendly Acceptable Ads ? Currently I'm linking to the same place where we host "old" acceptable-ads information. Probably make sense to remove the link completely meanwhile ?
On 2017/10/05 22:17:49, saroyanm wrote: > https://codereview.adblockplus.org/29565858/diff/29565859/pages/preferences.tmpl > File pages/preferences.tmpl (right): > > https://codereview.adblockplus.org/29565858/diff/29565859/pages/preferences.t... > pages/preferences.tmpl:228: {{"Location of the filter list used to\n<a > href=\"acceptable-ads\">allow privacy-friendly non-intrusive > ads</a>."|translate("subscriptions_exceptionsurl-privacy")}} > @Wspee: DO we have, or planing to have a dedicated page for describing Privacy > Friendly Acceptable Ads ? > > Currently I'm linking to the same place where we host "old" acceptable-ads > information. > > Probably make sense to remove the link completely meanwhile ? We plan to add a paragraph on >https://adblockplus.org/en/acceptable-ads>. I would suggest to use the documentation link `acceptable_ads` for now (would be ship-able if need be this way), but it probably make sense to add a dedicated documentation link once the paragraph is up.
On 2017/10/06 08:08:59, wspee wrote: > On 2017/10/05 22:17:49, saroyanm wrote: > > > https://codereview.adblockplus.org/29565858/diff/29565859/pages/preferences.tmpl > > File pages/preferences.tmpl (right): > > > > > https://codereview.adblockplus.org/29565858/diff/29565859/pages/preferences.t... > > pages/preferences.tmpl:228: {{"Location of the filter list used to\n<a > > href=\"acceptable-ads\">allow privacy-friendly non-intrusive > > ads</a>."|translate("subscriptions_exceptionsurl-privacy")}} > > @Wspee: DO we have, or planing to have a dedicated page for describing Privacy > > Friendly Acceptable Ads ? > > > > Currently I'm linking to the same place where we host "old" acceptable-ads > > information. > > > > Probably make sense to remove the link completely meanwhile ? > > We plan to add a paragraph on >https://adblockplus.org/en/acceptable-ads>. > > I would suggest to use the documentation link `acceptable_ads` for now (would be > ship-able if need be this way), but it probably make sense to add a dedicated > documentation link once the paragraph is up. (Not sure if this part also uses documentation links, if not use the link it-self of course ;))
On 2017/10/06 08:14:39, wspee wrote: > On 2017/10/06 08:08:59, wspee wrote: > > On 2017/10/05 22:17:49, saroyanm wrote: > > > > > > https://codereview.adblockplus.org/29565858/diff/29565859/pages/preferences.tmpl > > > File pages/preferences.tmpl (right): > > > > > > > > > https://codereview.adblockplus.org/29565858/diff/29565859/pages/preferences.t... > > > pages/preferences.tmpl:228: {{"Location of the filter list used to\n<a > > > href=\"acceptable-ads\">allow privacy-friendly non-intrusive > > > ads</a>."|translate("subscriptions_exceptionsurl-privacy")}} > > > @Wspee: DO we have, or planing to have a dedicated page for describing > Privacy > > > Friendly Acceptable Ads ? > > > > > > Currently I'm linking to the same place where we host "old" acceptable-ads > > > information. > > > > > > Probably make sense to remove the link completely meanwhile ? > > > > We plan to add a paragraph on >https://adblockplus.org/en/acceptable-ads>. > > > > I would suggest to use the documentation link `acceptable_ads` for now (would > be > > ship-able if need be this way), but it probably make sense to add a dedicated > > documentation link once the paragraph is up. > > (Not sure if this part also uses documentation links, if not use the link > it-self of course ;)) As discussed in person: We don't need to have link here at all and keep the wording consistent. We will add the link here as soon we will create a section defecated to the Privacy friendly acceptable ads. New patch uploaded and ready for the review.
Thanks Manvel. Just one comment https://codereview.adblockplus.org/29565858/diff/29567796/pages/preferences.tmpl File pages/preferences.tmpl (right): https://codereview.adblockplus.org/29565858/diff/29567796/pages/preferences.t... pages/preferences.tmpl:557: "name": "subscriptions_exceptionsurl_privacy", This name doesn't match the translation id used above `subscriptions_exceptionsurl-privacy` vs `subscriptions_exceptionsurl_privacy` (you used a `-` instead of `_` above)
New patch uploaded. https://codereview.adblockplus.org/29565858/diff/29567796/pages/preferences.tmpl File pages/preferences.tmpl (right): https://codereview.adblockplus.org/29565858/diff/29567796/pages/preferences.t... pages/preferences.tmpl:557: "name": "subscriptions_exceptionsurl_privacy", On 2017/10/09 08:45:16, ire wrote: > This name doesn't match the translation id used above > > `subscriptions_exceptionsurl-privacy` vs `subscriptions_exceptionsurl_privacy` > > (you used a `-` instead of `_` above) Yes it's not, It's the name which appears in the preferences page and used by extension, but not the one that's used as a translation string. To be consistent with other translation strings in this page I think we should use "subscriptions_exceptionsurlPrivacy" instead for translation string. If we will decide to use "name" as a string ID I think we should adapt the macroses accordingly, but that's out of the scope of current review I think. Side note: Also "-s1" in the end of the strings are leftover from the CMS migration while ago, where we migrated all strings from the old CMS called anwiki.
LGTM https://codereview.adblockplus.org/29565858/diff/29567796/pages/preferences.tmpl File pages/preferences.tmpl (right): https://codereview.adblockplus.org/29565858/diff/29567796/pages/preferences.t... pages/preferences.tmpl:557: "name": "subscriptions_exceptionsurl_privacy", On 2017/10/09 11:07:44, saroyanm wrote: > On 2017/10/09 08:45:16, ire wrote: > > This name doesn't match the translation id used above > > > > `subscriptions_exceptionsurl-privacy` vs `subscriptions_exceptionsurl_privacy` > > > > (you used a `-` instead of `_` above) > > Yes it's not, It's the name which appears in the preferences page and used by > extension, but not the one that's used as a translation string. > > To be consistent with other translation strings in this page I think we should > use "subscriptions_exceptionsurlPrivacy" instead for translation string. > > If we will decide to use "name" as a string ID I think we should adapt the > macroses accordingly, but that's out of the scope of current review I think. > > Side note: Also "-s1" in the end of the strings are leftover from the CMS > migration while ago, where we migrated all strings from the old CMS called > anwiki. Oh okay. My bad, I thought there were supposed to match. Your modification makes things clearer, thanks 
|
