Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(143)

Issue 29556681: Issue 5779 - Missing checkbox in language dialog and persistent validation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by saroyanm
Modified:
1 year, 11 months ago
Reviewers:
ire
CC:
Thomas Greiner
Visibility:
Public.

Description

Issue 5779 - Missing checkbox in language dialog and persistent validation

Patch Set 1 #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -8 lines) Patch
M new-options.html View 2 chunks +3 lines, -3 lines 2 comments Download
M new-options.js View 1 chunk +2 lines, -2 lines 0 comments Download
M skin/new-options.css View 4 chunks +4 lines, -3 lines 4 comments Download

Messages

Total messages: 3
saroyanm
@Ire can you please have a look on this review as well please. https://codereview.adblockplus.org/29556681/diff/29556682/new-options.html File ...
1 year, 11 months ago (2017-09-26 18:40:52 UTC) #1
ire
> @Ire can you please have a look on this review as well please. Thanks ...
1 year, 11 months ago (2017-09-28 07:33:44 UTC) #2
saroyanm
1 year, 11 months ago (2017-09-28 11:55:06 UTC) #3
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5