Issue 5374 - Add filter list popup

This review contains HTML/Semantics, strings and functional implementation as specified in (https://bitbucket.org/adblockplus/spec/src/6da96ae41ce19186730b1459364cb6de54345c9e/spec/abp/options-page.md?at=master&fileviewer=file-view-default#markdown-header-add-filter-list-popup) and small CSS to visually represent the layout. CSS of whole options page will be revisited and reimplemented separately.

[saroyanm, 2017-07-07 18:22:11]

Ready for the review.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.js#newc...
new-options.js:463: subscription.description = getMessage("options_language_" +
prefix);
The strings will need to be updated after the specification update, see ->
https://bitbucket.org/adblockplus/spec/issues/44

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.js#newc...
new-options.js:470: subscription.description = getMessage("options_feature_" +
I don't think that TYPE is a unique identifier for the recommended
subscriptions, we shouldn't use type for the string assignment.
I think we should introduce a new attribute ? Maybe 'referenceId' ?

[saroyanm, 2017-07-10 13:00:06]

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html
File new-options.html (right):

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:376: <ul id="recommend-general-list-table" class="table list">
As discussed already, we do not need to introduce current list for the initial
release. This list should be removed from the review.

[saroyanm, 2017-07-13 17:09:22]

This is still ready for the review.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html
File new-options.html (right):

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:376: <ul id="recommend-general-list-table" class="table list">
On 2017/07/10 13:00:05, saroyanm wrote:
> As discussed already, we do not need to introduce current list for the initial
> release. This list should be removed from the review.

Product, agreed on introducing this anyway if it's easy to implement, which
looks to be easy.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.js#newc...
new-options.js:463: subscription.description = getMessage("options_language_" +
prefix);
On 2017/07/07 18:22:11, saroyanm wrote:
> The strings will need to be updated after the specification update, see ->
> https://bitbucket.org/adblockplus/spec/issues/44

Updated the reference -> https://bitbucket.org/adblockplus/spec/issues/47

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.js#newc...
new-options.js:470: subscription.description = getMessage("options_feature_" +
On 2017/07/07 18:22:11, saroyanm wrote:
> I don't think that TYPE is a unique identifier for the recommended
> subscriptions, we shouldn't use type for the string assignment.
> I think we should introduce a new attribute ? Maybe 'referenceId' ?

As discussed we can still use type, while we are planing to introduce 1 type per
recommendation. which makes unique "for now?".

[Thomas Greiner, 2017-07-25 10:30:55]

https://codereview.adblockplus.org/29480624/diff/29482717/locale/en-US/new-op...
File locale/en-US/new-options.json (right):

https://codereview.adblockplus.org/29480624/diff/29482717/locale/en-US/new-op...
locale/en-US/new-options.json:456: "options_feature_anti_adblock_title": {
Those are the names of the filter lists so no need to translate them -
especially given that the filter lists behind the features may change at any
point.

https://codereview.adblockplus.org/29480624/diff/29482717/locale/en-US/new-op...
locale/en-US/new-options.json:456: "options_feature_anti_adblock_title": {
According to
https://bitbucket.org/adblockplus/spec/src/6eb0fc6e906a60ac76d93cf1ba9ae770e0...
we should only display the "privacy" and "social" filter lists.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html
File new-options.html (right):

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:376: <ul id="recommend-general-list-table" class="table list">
On 2017/07/13 17:09:22, saroyanm wrote:
> On 2017/07/10 13:00:05, saroyanm wrote:
> > As discussed already, we do not need to introduce current list for the
initial
> > release. This list should be removed from the review.
> 
> Product, agreed on introducing this anyway if it's easy to implement, which
> looks to be easy.

It's no longer in the spec, however.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:378: <button data-action="add-enable-subscription"
role="checkbox" class="control"></button>
The entire row should be clickable, not just the invisible checkmark and the
text. Furthermore, the checkmark is merely an indicator of the status of the
list item.

So semantically, you could, for instance, make the entire row be a button.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:394: <button class="i18n_options_filter_by_url"
data-action="close-dialog,open-dialog" data-dialog="custom"></button>
I'd argue that "open-dialog" should automatically close any existing dialog.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:396: <!-- Add language subscription -->
I thought we wanted to replace this dialog with the new one already?

https://codereview.adblockplus.org/29480624/diff/29482717/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29480624/diff/29482717/skin/new-options.cs...
skin/new-options.css:1178: #recommended-general-ads-block
Detail: That's a very specific selector for a pretty generic scrollable element.
So what about turning this into a class for making an element scrollable?

https://codereview.adblockplus.org/29480624/diff/29482717/skin/new-options.cs...
skin/new-options.css:1180: max-height: 400px;
Detail: I couldn't find this number anywhere in the spec. Where does it come
from?

[saroyanm, 2017-07-28 17:54:55]

https://codereview.adblockplus.org/29480624/diff/29482717/locale/en-US/new-op...
File locale/en-US/new-options.json (right):

https://codereview.adblockplus.org/29480624/diff/29482717/locale/en-US/new-op...
locale/en-US/new-options.json:456: "options_feature_anti_adblock_title": {
On 2017/07/25 10:30:54, Thomas Greiner wrote:
> According to
>
https://bitbucket.org/adblockplus/spec/src/6eb0fc6e906a60ac76d93cf1ba9ae770e0...
> we should only display the "privacy" and "social" filter lists.

We don't need to display them in current Dialog, so this should be removed

https://codereview.adblockplus.org/29480624/diff/29482717/locale/en-US/new-op...
locale/en-US/new-options.json:456: "options_feature_anti_adblock_title": {
On 2017/07/25 10:30:54, Thomas Greiner wrote:
> Those are the names of the filter lists so no need to translate them -
> especially given that the filter lists behind the features may change at any
> point.

I agree, but we don't need to display them in current Dialog, so this should be
removed.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html
File new-options.html (right):

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:376: <ul id="recommend-general-list-table" class="table list">
On 2017/07/25 10:30:54, Thomas Greiner wrote:
> On 2017/07/13 17:09:22, saroyanm wrote:
> > On 2017/07/10 13:00:05, saroyanm wrote:
> > > As discussed already, we do not need to introduce current list for the
> initial
> > > release. This list should be removed from the review.
> > 
> > Product, agreed on introducing this anyway if it's easy to implement, which
> > looks to be easy.
> 
> It's no longer in the spec, however.

I'll remove this.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:378: <button data-action="add-enable-subscription"
role="checkbox" class="control"></button>
On 2017/07/25 10:30:54, Thomas Greiner wrote:
> The entire row should be clickable, not just the invisible checkmark and the
> text. Furthermore, the checkmark is merely an indicator of the status of the
> list item.
> 
> So semantically, you could, for instance, make the entire row be a button.

I agree, I'll update it in current review.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:394: <button class="i18n_options_filter_by_url"
data-action="close-dialog,open-dialog" data-dialog="custom"></button>
On 2017/07/25 10:30:54, Thomas Greiner wrote:
> I'd argue that "open-dialog" should automatically close any existing dialog.

I agree, I'll add dialog management in current review.

https://codereview.adblockplus.org/29480624/diff/29482717/new-options.html#ne...
new-options.html:396: <!-- Add language subscription -->
On 2017/07/25 10:30:54, Thomas Greiner wrote:
> I thought we wanted to replace this dialog with the new one already?

What you mean by new one ?
Sidenote: This is different Dialog than the one targeted in the review, this
dialog will be updated in general tab review.

https://codereview.adblockplus.org/29480624/diff/29482717/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29480624/diff/29482717/skin/new-options.cs...
skin/new-options.css:1178: #recommended-general-ads-block
On 2017/07/25 10:30:54, Thomas Greiner wrote:
> Detail: That's a very specific selector for a pretty generic scrollable
element.
> So what about turning this into a class for making an element scrollable?

I'll update that.

https://codereview.adblockplus.org/29480624/diff/29482717/skin/new-options.cs...
skin/new-options.css:1180: max-height: 400px;
On 2017/07/25 10:30:54, Thomas Greiner wrote:
> Detail: I couldn't find this number anywhere in the spec. Where does it come
> from?

CSS will be updated completely in separate review according to styleguide.
This styles are only for visually make the list scrollable.

[saroyanm, 2017-07-28 18:08:11]

Having another look, this is NO LGTM.

This issue is dependant on General Tab, as the add language dialog and dialog
here are almost identical and the difference is in the additional button below
the dialog. I'll closing this review and will follow on this after General Tab
review is ready.

[saroyanm, 2017-07-28 18:08:28]

On 2017/07/28 18:08:11, saroyanm wrote:
> Having another look, this is NO LGTM.
> 
> This issue is dependant on General Tab, as the add language dialog and dialog
> here are almost identical and the difference is in the additional button below
> the dialog. I'll closing this review and will follow on this after General Tab
> review is ready.

NOT LGTM