Issue 5779 - Missing checkbox in language dialog and persistent validation

Issue 5779 - Missing checkbox in language dialog and persistent validation

[saroyanm, 2017-09-26 18:40:52]

@Ire can you please have a look on this review as well please.

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

https://codereview.adblockplus.org/29556681/diff/29556682/new-options.html#ne...
new-options.html:350: <button class="i18n_options_close primary default-focus"
data-action="close-dialog"></button>
This dialogs were missing default focus, I thought it's a trivial change that we
can fix here as well.

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

https://codereview.adblockplus.org/29556681/diff/29556682/skin/new-options.cs...
skin/new-options.css:352: [data-validation] .floating-input input:focus:invalid
~ .attention::before,
Making invalid errors being persistent with current implementation is not
trivial.
The only way with current implementation I could have done it is by checking if
:placeholder-shown persist, which is true whenever the input is empty.

Anyway Jeen initial concern was about valid inputs being persistent.
I'll double check with her regarding the invalid inputs, but for the scope of
this issue having only valid inputs persistent I think should be fine.

https://codereview.adblockplus.org/29556681/diff/29556682/skin/new-options.cs...
skin/new-options.css:1140: font-weight: 700;
Font-weight context menu items were different and Update and Delete button were
looking emphasized, so I made that consistent as well within this issue.

[ire, 2017-09-28 07:33:44]

> @Ire can you please have a look on this review as well please.

Thanks Manvel! LGTM

This is for another issue, but I think the "checked" state for the languages in
the "add/update language" dialog should make the input disabled/non-clickable.
Because clicking them doesn't really do anything besides close the dialog. An
actual checkbox should allow you to "uncheck" by clicking on it again, and since
this doesn't do that, I think the checked state should also be disabled. Or,
ideally, make it a proper "checkbox" pattern, which would include: allowing the
user to "check" on multiple items at once as well as "uncheck".

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

https://codereview.adblockplus.org/29556681/diff/29556682/new-options.html#ne...
new-options.html:350: <button class="i18n_options_close primary default-focus"
data-action="close-dialog"></button>
On 2017/09/26 18:40:52, saroyanm wrote:
> This dialogs were missing default focus, I thought it's a trivial change that
we
> can fix here as well.

Acknowledged.

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

https://codereview.adblockplus.org/29556681/diff/29556682/skin/new-options.cs...
skin/new-options.css:352: [data-validation] .floating-input input:focus:invalid
~ .attention::before,
On 2017/09/26 18:40:52, saroyanm wrote:
> Making invalid errors being persistent with current implementation is not
> trivial.
> The only way with current implementation I could have done it is by checking
if
> :placeholder-shown persist, which is true whenever the input is empty.
> 
> Anyway Jeen initial concern was about valid inputs being persistent.
> I'll double check with her regarding the invalid inputs, but for the scope of
> this issue having only valid inputs persistent I think should be fine.

Acknowledged.

https://codereview.adblockplus.org/29556681/diff/29556682/skin/new-options.cs...
skin/new-options.css:1140: font-weight: 700;
On 2017/09/26 18:40:52, saroyanm wrote:
> Font-weight context menu items were different and Update and Delete button
were
> looking emphasized, so I made that consistent as well within this issue.

Acknowledged.

[saroyanm, 2017-09-28 11:55:06]

On 2017/09/28 07:33:44, ire wrote:
> > @Ire can you please have a look on this review as well please.
> 
> Thanks Manvel! LGTM
> 
> This is for another issue, but I think the "checked" state for the languages
in
> the "add/update language" dialog should make the input disabled/non-clickable.
> Because clicking them doesn't really do anything besides close the dialog. An
> actual checkbox should allow you to "uncheck" by clicking on it again, and
since
> this doesn't do that, I think the checked state should also be disabled. Or,
> ideally, make it a proper "checkbox" pattern, which would include: allowing
the
> user to "check" on multiple items at once as well as "uncheck".
Yes you are right I think I'll need to introduce additional preference for
Collections(aka tables).
I've created an issue to tackled that separately ->
https://issues.adblockplus.org/ticket/5804

> https://codereview.adblockplus.org/29556681/diff/29556682/new-options.html
> File new-options.html (right):
> 
>
https://codereview.adblockplus.org/29556681/diff/29556682/new-options.html#ne...
> new-options.html:350: <button class="i18n_options_close primary default-focus"
> data-action="close-dialog"></button>
> On 2017/09/26 18:40:52, saroyanm wrote:
> > This dialogs were missing default focus, I thought it's a trivial change
that
> we
> > can fix here as well.
> 
> Acknowledged.
> 
> https://codereview.adblockplus.org/29556681/diff/29556682/skin/new-options.css
> File skin/new-options.css (right):
> 
>
https://codereview.adblockplus.org/29556681/diff/29556682/skin/new-options.cs...
> skin/new-options.css:352: [data-validation] .floating-input
input:focus:invalid
> ~ .attention::before,
> On 2017/09/26 18:40:52, saroyanm wrote:
> > Making invalid errors being persistent with current implementation is not
> > trivial.
> > The only way with current implementation I could have done it is by checking
> if
> > :placeholder-shown persist, which is true whenever the input is empty.
> > 
> > Anyway Jeen initial concern was about valid inputs being persistent.
> > I'll double check with her regarding the invalid inputs, but for the scope
of
> > this issue having only valid inputs persistent I think should be fine.
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29556681/diff/29556682/skin/new-options.cs...
> skin/new-options.css:1140: font-weight: 700;
> On 2017/09/26 18:40:52, saroyanm wrote:
> > Font-weight context menu items were different and Update and Delete button
> were
> > looking emphasized, so I made that consistent as well within this issue.
> 
> Acknowledged.