Issue 5706 - Finish custom filter list dialog

Issue 5706 - Finish dd custom filter list dialog

[saroyanm, 2017-09-19 18:53:03]

This is ready for review.

@Julian can you please have a look ?

@Thomas If you will have a chance to check my form validation implementation
against the specification I think that will be also helpful.

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

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.js#newc...
new-options.js:857: form.querySelector(":invalid").focus();
I need to select first invalid input

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

https://codereview.adblockplus.org/29549891/diff/29549897/skin/new-options.cs...
skin/new-options.css:281: .side-controls button
Note: Now we have controls that are pushed to the end in multiple places, for
that reason I've created .side-controls class to push controls and nowrap to
make them being align on same line.

https://codereview.adblockplus.org/29549891/diff/29549897/skin/new-options.cs...
skin/new-options.css:384: background-color: green;
We don't have green color yet, see ->
https://bitbucket.org/adblockplus/spec/issues/79/valid-icon-is-missing

[ire, 2017-09-20 14:53:13]

Thanks Manvel! Comments below

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

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:367: <form data-validation="custom" action="">
NIT: Why define the action if there's no action?

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:369: <input placeholder=" " id="import-list-title" type="text"
class="default-focus" required />
NIT: Why define a placeholder if it's just an empty space?

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:370: <label for="import-list-title"
class="i18n_options_dialog_import_subscription_title"></label>
For some reason, this label doesn't move to the place of a placeholder when
focus is on another input and this input is empty

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:380: <p class="side-controls nowrap">
Suggestion/NIT: Perhaps the default should be `nowrap` and you instead create a
class for `wrap`?

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

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.js#newc...
new-options.js:688: }
I'm not sure why we need the eventListener on the forms you added below, when we
can check for the form being invalid here in the `else` part of this statement.
The only issue I see with that is in calling `e.preventDefault()`, since the
event is not passed to this `exec` function (which it could be added).

The reason I'm bringing this up is because it may be unnecessary to add event
listeners to *every* `[data-validation]` form, when there is already an event
listener added for the submit of the form (via the click of the submit button in
this case).

Although your method has the benefit of being extensible to other forms that may
need validation, the logic behind that could be extracted without the need to
add the event listeners to every form.

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.js#newc...
new-options.js:845: document.body.addEventListener("submit", (e) =>
The "submit" event listener is only relevant to forms. I don't think this is
necessary or does anything

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.js#newc...
new-options.js:857: form.querySelector(":invalid").focus();
On 2017/09/19 18:53:02, saroyanm wrote:
> I need to select first invalid input

Acknowledged.

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

https://codereview.adblockplus.org/29549891/diff/29549897/skin/new-options.cs...
skin/new-options.css:281: .side-controls button
On 2017/09/19 18:53:02, saroyanm wrote:
> Note: Now we have controls that are pushed to the end in multiple places, for
> that reason I've created .side-controls class to push controls and nowrap to
> make them being align on same line.

Acknowledged.

https://codereview.adblockplus.org/29549891/diff/29549897/skin/new-options.cs...
skin/new-options.css:384: background-color: green;
On 2017/09/19 18:53:02, saroyanm wrote:
> We don't have green color yet, see ->
> https://bitbucket.org/adblockplus/spec/issues/79/valid-icon-is-missing

Acknowledged.

[saroyanm, 2017-09-21 19:07:54]

Thanks for the comments Ire.

New patch is uploaded.

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

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:367: <form data-validation="custom" action="">
On 2017/09/20 14:53:12, ire wrote:
> NIT: Why define the action if there's no action?

Done.

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:369: <input placeholder=" " id="import-list-title" type="text"
class="default-focus" required />
On 2017/09/20 14:53:12, ire wrote:
> NIT: Why define a placeholder if it's just an empty space?

So I could keep the label on top of the input when the input is not empty, I can
select this way input in "non empty" state with CSS.

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:370: <label for="import-list-title"
class="i18n_options_dialog_import_subscription_title"></label>
On 2017/09/20 14:53:12, ire wrote:
> For some reason, this label doesn't move to the place of a placeholder when
> focus is on another input and this input is empty

I can not reproduce this, can you please let me know the reproduction
environment are you using?

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:380: <p class="side-controls nowrap">
On 2017/09/20 14:53:12, ire wrote:
> Suggestion/NIT: Perhaps the default should be `nowrap` and you instead create
a
> class for `wrap`?

Done.

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

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.js#newc...
new-options.js:688: }
On 2017/09/20 14:53:12, ire wrote:
> I'm not sure why we need the eventListener on the forms you added below, when
we
> can check for the form being invalid here in the `else` part of this
statement.
> The only issue I see with that is in calling `e.preventDefault()`, since the
> event is not passed to this `exec` function (which it could be added).
> 
> The reason I'm bringing this up is because it may be unnecessary to add event
> listeners to *every* `[data-validation]` form, when there is already an event
> listener added for the submit of the form (via the click of the submit button
in
> this case).
> 
> Although your method has the benefit of being extensible to other forms that
may
> need validation, the logic behind that could be extracted without the need to
> add the event listeners to every form. 

You are absolutely right. The reason was because I wanted to prevent default on
all forms submission, while all other code should be here.
Not to complicate this with the submission issue, I'll not use type="submit".
Because if we would like to handle this on submission we probably need to create
a submit handling function, where we might want to get action from the form
element and I think that can be tackled here ->
https://issues.adblockplus.org/ticket/5251 .
Anyway, done :)

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.js#newc...
new-options.js:845: document.body.addEventListener("submit", (e) =>
On 2017/09/20 14:53:13, ire wrote:
> The "submit" event listener is only relevant to forms. I don't think this is
> necessary or does anything

Well spotted, I Forgot to delete this.
Done.

[ire, 2017-09-22 08:28:54]

Thanks again Manvel. This LGTM

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

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:369: <input placeholder=" " id="import-list-title" type="text"
class="default-focus" required />
On 2017/09/21 19:07:53, saroyanm wrote:
> On 2017/09/20 14:53:12, ire wrote:
> > NIT: Why define a placeholder if it's just an empty space?
> 
> So I could keep the label on top of the input when the input is not empty, I
can
> select this way input in "non empty" state with CSS.

Ah I see! Ack.

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.html#ne...
new-options.html:370: <label for="import-list-title"
class="i18n_options_dialog_import_subscription_title"></label>
On 2017/09/21 19:07:53, saroyanm wrote:
> On 2017/09/20 14:53:12, ire wrote:
> > For some reason, this label doesn't move to the place of a placeholder when
> > focus is on another input and this input is empty
> 
> I can not reproduce this, can you please let me know the reproduction
> environment are you using?

I realised that I had removed the `placeholder` attribute on the the field,
that's why it wasn't working.

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

https://codereview.adblockplus.org/29549891/diff/29549897/new-options.js#newc...
new-options.js:688: }
On 2017/09/21 19:07:54, saroyanm wrote:
> On 2017/09/20 14:53:12, ire wrote:
> > I'm not sure why we need the eventListener on the forms you added below,
when
> we
> > can check for the form being invalid here in the `else` part of this
> statement.
> > The only issue I see with that is in calling `e.preventDefault()`, since the
> > event is not passed to this `exec` function (which it could be added).
> > 
> > The reason I'm bringing this up is because it may be unnecessary to add
event
> > listeners to *every* `[data-validation]` form, when there is already an
event
> > listener added for the submit of the form (via the click of the submit
button
> in
> > this case).
> > 
> > Although your method has the benefit of being extensible to other forms that
> may
> > need validation, the logic behind that could be extracted without the need
to
> > add the event listeners to every form. 
> 
> You are absolutely right. The reason was because I wanted to prevent default
on
> all forms submission, while all other code should be here.
> Not to complicate this with the submission issue, I'll not use type="submit".
> Because if we would like to handle this on submission we probably need to
create
> a submit handling function, where we might want to get action from the form
> element and I think that can be tackled here ->
> https://issues.adblockplus.org/ticket/5251 .
> Anyway, done :) 

Got it :)

As long as the button will be changed to a submit in #5251, that's fine by me!

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

https://codereview.adblockplus.org/29549891/diff/29551700/new-options.js#newc...
new-options.js:687: addEnableSubscription(E("import-list-url").value,
NIT: I'm getting an eslint error here: "no-trailing-spaces"

[saroyanm, 2017-09-22 09:46:59]

Awesome, thanks!

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

https://codereview.adblockplus.org/29549891/diff/29551700/new-options.js#newc...
new-options.js:687: addEnableSubscription(E("import-list-url").value,
On 2017/09/22 08:28:53, ire wrote:
> NIT: I'm getting an eslint error here: "no-trailing-spaces"

Done.