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

Issue 29411555: Issue 5169 - Add whitelisted tab to the new options page

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks ago by saroyanm
Modified:
6 days, 14 hours ago
Reviewers:
Thomas Greiner
CC:
wspee
Visibility:
Public.

Description

I think styles need to be implemented separately, because I think most of them might need to be applied globally (Layout, Table, Form elements)

Patch Set 1 : #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -92 lines) Patch
M locale/en-US/new-options.json View 2 chunks +40 lines, -4 lines 0 comments Download
M new-options.html View 2 chunks +29 lines, -33 lines 2 comments Download
M new-options.js View 5 chunks +71 lines, -18 lines 2 comments Download
M skin/new-options.css View 2 chunks +1 line, -37 lines 0 comments Download

Messages

Total messages: 1
saroyanm
6 days, 14 hours ago (2017-04-21 12:03:17 UTC) #1
Ready for review, I added couple of comments so we can start discuss some
questions.

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

https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne...
new-options.html:177: <div>
I think this should be a <form> element and button type="submit".
Same applies any other form control elements that "submit" data.

https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne...
new-options.html:180: data-action="add-domain-exception"
I think this control element should be changed as part of global redesign of
Removable tables.

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

https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc...
new-options.js:791: let validIpAddressRegex =
"^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25"
Regular expression to match Hostname and IP addresses as specified:
http://stackoverflow.com/questions/106179/regular-expression-to-match-dns-hos...

https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc...
new-options.js:805: addWhitelistButton.setAttribute("disabled", "");
I think we should use checkValidity() method instead and implement all the
checks on HTML element including RegExp also implement <form> as commented in
options.html.
Sign in to reply to this message.

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