Issue 29339284: Issue 3884 - Show original subscription title in the Advanced tab of the options page (Closed)
|
Created:
April 1, 2016, 2:58 p.m. by Sebastian Noack Modified:
April 1, 2016, 5:30 p.m. Visibility:
Public. |
DescriptionIssue 3884 - Show original subscription title in the Advanced tab of the options page
Patch Set 1 #
Total comments: 4
MessagesTotal messages: 8
https://codereview.adblockplus.org/29339284/diff/29339285/options.js File options.js (right): https://codereview.adblockplus.org/29339284/diff/29339285/options.js#newcode856 options.js:856: subscription.title = getMessage("options_acceptableAds_description"); I think we also should specify original title here.
https://codereview.adblockplus.org/29339284/diff/29339285/options.js File options.js (right): https://codereview.adblockplus.org/29339284/diff/29339285/options.js#newcode856 options.js:856: subscription.title = getMessage("options_acceptableAds_description"); On 2016/04/01 15:17:50, saroyanm wrote: > I think we also should specify original title here. If I don't miss something we don't have the original title here. Not that it matters much for this subscription.
https://codereview.adblockplus.org/29339284/diff/29339285/options.js File options.js (right): https://codereview.adblockplus.org/29339284/diff/29339285/options.js#newcode856 options.js:856: subscription.title = getMessage("options_acceptableAds_description"); On 2016/04/01 15:20:37, Sebastian Noack wrote: > On 2016/04/01 15:17:50, saroyanm wrote: > > I think we also should specify original title here. > > If I don't miss something we don't have the original title here. Not that it > matters much for this subscription. Well, currently AA title is "Allow non-intrusive advertising" in filter list, while we using "Allow some non-intrusive advertising", also this will be only item in advanced tab that will be translatable with current implementation. So I think this should behave same way as other subscriptions, not sure why this item should behave differently.
https://codereview.adblockplus.org/29339284/diff/29339285/options.js File options.js (right): https://codereview.adblockplus.org/29339284/diff/29339285/options.js#newcode856 options.js:856: subscription.title = getMessage("options_acceptableAds_description"); On 2016/04/01 15:33:54, saroyanm wrote: > On 2016/04/01 15:20:37, Sebastian Noack wrote: > > On 2016/04/01 15:17:50, saroyanm wrote: > > > I think we also should specify original title here. > > > > If I don't miss something we don't have the original title here. Not that it > > matters much for this subscription. > > Well, currently AA title is "Allow non-intrusive advertising" in filter list, > while we using "Allow some non-intrusive advertising", also this will be only > item in advanced tab that will be translatable with current implementation. So I > think this should behave same way as other subscriptions, not sure why this item > should behave differently. Well, "Own filter list", referring to the SpecialSubscription for the custom filters, is also translated in that list. Also one could argue that "Allow some non-intrusive advertising" should be consistently translated, since the name only differs in a single word as opposed to the other aliased/translated subscription titles. I don't have a strong opinion though, though I mildly lend towards leaving it as it is, if it's just to keep it simple.
On 2016/04/01 15:55:02, Sebastian Noack wrote: > https://codereview.adblockplus.org/29339284/diff/29339285/options.js > File options.js (right): > > https://codereview.adblockplus.org/29339284/diff/29339285/options.js#newcode856 > options.js:856: subscription.title = > getMessage("options_acceptableAds_description"); > On 2016/04/01 15:33:54, saroyanm wrote: > > On 2016/04/01 15:20:37, Sebastian Noack wrote: > > > On 2016/04/01 15:17:50, saroyanm wrote: > > > > I think we also should specify original title here. > > > > > > If I don't miss something we don't have the original title here. Not that it > > > matters much for this subscription. > > > > Well, currently AA title is "Allow non-intrusive advertising" in filter list, > > while we using "Allow some non-intrusive advertising", also this will be only > > item in advanced tab that will be translatable with current implementation. So > I > > think this should behave same way as other subscriptions, not sure why this > item > > should behave differently. > > Well, "Own filter list", referring to the SpecialSubscription for the custom > filters, is also translated in that list. Also one could argue that "Allow some Yes it's also translated, but this item is exception I assume and it's can't have "Original title" because there can be several SpecialSubscription. > non-intrusive advertising" should be consistently translated, since the name > only differs in a single word as opposed to the other aliased/translated > subscription titles. I don't have a strong opinion though, though I mildly lend > towards leaving it as it is, if it's just to keep it simple. I see that it's easier to make another exception for another item with current implementation, but I would rather vote for adding "Original title" to this item as well to keep it as consistent as possible. What you think Thomas ? Mind giving third opinion ?
On 2016/04/01 16:10:54, saroyanm wrote: > On 2016/04/01 15:55:02, Sebastian Noack wrote: > > https://codereview.adblockplus.org/29339284/diff/29339285/options.js > > File options.js (right): > > > > > https://codereview.adblockplus.org/29339284/diff/29339285/options.js#newcode856 > > options.js:856: subscription.title = > > getMessage("options_acceptableAds_description"); > > On 2016/04/01 15:33:54, saroyanm wrote: > > > On 2016/04/01 15:20:37, Sebastian Noack wrote: > > > > On 2016/04/01 15:17:50, saroyanm wrote: > > > > > I think we also should specify original title here. > > > > > > > > If I don't miss something we don't have the original title here. Not that > it > > > > matters much for this subscription. > > > > > > Well, currently AA title is "Allow non-intrusive advertising" in filter > list, > > > while we using "Allow some non-intrusive advertising", also this will be > only > > > item in advanced tab that will be translatable with current implementation. > So > > I > > > think this should behave same way as other subscriptions, not sure why this > > item > > > should behave differently. > > > > Well, "Own filter list", referring to the SpecialSubscription for the custom > > filters, is also translated in that list. Also one could argue that "Allow > some > Yes it's also translated, but this item is exception I assume and it's can't > have "Original title" because there can be several SpecialSubscription. > > non-intrusive advertising" should be consistently translated, since the name > > only differs in a single word as opposed to the other aliased/translated > > subscription titles. I don't have a strong opinion though, though I mildly > lend > > towards leaving it as it is, if it's just to keep it simple. > I see that it's easier to make another exception for another item with current > implementation, but I would rather vote for adding "Original title" to this item > as well to keep it as consistent as possible. > What you think Thomas ? Mind giving third opinion ? Generally, Acceptable Ads is always a special case and since we don't use the title that's provided by the filter list in any of our other versions, we probably shouldn't use it here either but instead use the text provided by the extension. That being said, we already need to treat it differently since users shouldn't be able to disable it (see #3885). Disabling the checkbox is functionally sufficient for the moment but not a great UX yet. Later on we can probably visually separate it from the other filter lists and change the behavior of the checkbox to install/remove the filter list rather than enable/disable it (like we do it with the blocking options in the General tab). Therefore we could implement its title like with any other filter list for now and then change it later on when we figured out the final UI/UX for it.
On 2016/04/01 16:33:18, Thomas Greiner wrote: > On 2016/04/01 16:10:54, saroyanm wrote: > > On 2016/04/01 15:55:02, Sebastian Noack wrote: > > > https://codereview.adblockplus.org/29339284/diff/29339285/options.js > > > File options.js (right): > > > > > > > > > https://codereview.adblockplus.org/29339284/diff/29339285/options.js#newcode856 > > > options.js:856: subscription.title = > > > getMessage("options_acceptableAds_description"); > > > On 2016/04/01 15:33:54, saroyanm wrote: > > > > On 2016/04/01 15:20:37, Sebastian Noack wrote: > > > > > On 2016/04/01 15:17:50, saroyanm wrote: > > > > > > I think we also should specify original title here. > > > > > > > > > > If I don't miss something we don't have the original title here. Not > that > > it > > > > > matters much for this subscription. > > > > > > > > Well, currently AA title is "Allow non-intrusive advertising" in filter > > list, > > > > while we using "Allow some non-intrusive advertising", also this will be > > only > > > > item in advanced tab that will be translatable with current > implementation. > > So > > > I > > > > think this should behave same way as other subscriptions, not sure why > this > > > item > > > > should behave differently. > > > > > > Well, "Own filter list", referring to the SpecialSubscription for the > custom > > > filters, is also translated in that list. Also one could argue that "Allow > > some > > Yes it's also translated, but this item is exception I assume and it's can't > > have "Original title" because there can be several SpecialSubscription. > > > non-intrusive advertising" should be consistently translated, since the name > > > only differs in a single word as opposed to the other aliased/translated > > > subscription titles. I don't have a strong opinion though, though I mildly > > lend > > > towards leaving it as it is, if it's just to keep it simple. > > I see that it's easier to make another exception for another item with current > > implementation, but I would rather vote for adding "Original title" to this > item > > as well to keep it as consistent as possible. > > What you think Thomas ? Mind giving third opinion ? > > Generally, Acceptable Ads is always a special case and since we don't use the > title that's provided by the filter list in any of our other versions, we > probably shouldn't use it here either but instead use the text provided by the > extension. > > That being said, we already need to treat it differently since users shouldn't > be able to disable it (see #3885). Disabling the checkbox is functionally > sufficient for the moment but not a great UX yet. Later on we can probably > visually separate it from the other filter lists and change the behavior of the > checkbox to install/remove the filter list rather than enable/disable it (like > we do it with the blocking options in the General tab). > > Therefore we could implement its title like with any other filter list for now > and then change it later on when we figured out the final UI/UX for it. I see, so AA is another exception, agree on that, didn't realised before commenting regaridng that. LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||
