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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by saroyanm
Modified:
1 month, 2 weeks 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: 44

Patch Set 2 : Addressed Thomas comments #

Total comments: 15

Patch Set 3 : Rebased to changeset #109 #

Total comments: 1

Patch Set 4 : Addressed latest comments #

Total comments: 15

Patch Set 5 : Addressed comments #

Total comments: 14

Patch Set 6 : Move new domains to top, implicit submission and comments #

Total comments: 5

Patch Set 7 : Fixed the TYPO #

Unified diffs Side-by-side diffs Delta from patch set Stats (+130 lines, -144 lines) Patch
M locale/en-US/new-options.json View 1 2 3 4 5 4 chunks +37 lines, -27 lines 0 comments Download
M new-options.html View 1 2 3 4 2 chunks +28 lines, -33 lines 0 comments Download
M new-options.js View 1 2 3 4 5 6 14 chunks +62 lines, -35 lines 0 comments Download
M skin/new-options.css View 1 2 3 4 chunks +3 lines, -49 lines 0 comments Download
R skin/tooltips/whitelisted.png View 1 Binary file 0 comments Download

Messages

Total messages: 23
saroyanm
Ready for review, I added couple of comments so we can start discuss some questions. ...
4 months ago (2017-04-21 12:03:17 UTC) #1
Thomas Greiner
https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-options.json File locale/en-US/new-options.json (left): https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-options.json#oldcode134 locale/en-US/new-options.json:134: "options_whitelisted_add": { Detail: This doesn't appear to be used ...
3 months, 2 weeks ago (2017-05-09 13:42:56 UTC) #2
saroyanm
Addressed the comments, still need to find solution for adding the duplicate, whitelist item on ...
3 months, 1 week ago (2017-05-16 20:20:07 UTC) #3
saroyanm
https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-options.json#newcode140 locale/en-US/new-options.json:140: "message": "Whitelisted websites" I think we can reuse, options_tab_whitelist ...
3 months, 1 week ago (2017-05-17 13:15:16 UTC) #4
saroyanm
https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html#newcode176 new-options.html:176: <hr> Giving another thought on HR usage, we can ...
3 months, 1 week ago (2017-05-17 13:35:41 UTC) #5
saroyanm
https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html#newcode176 new-options.html:176: <hr> On 2017/05/17 13:35:41, saroyanm wrote: > Giving another ...
3 months, 1 week ago (2017-05-17 13:47:48 UTC) #6
saroyanm
Notes from the Review Meeting. 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#newcode780 new-options.js:780: E("whitelisting-textbox").addEventListener("keyup", (e) => On ...
3 months ago (2017-05-18 16:21:52 UTC) #7
saroyanm
Ready for the review https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-options.json#newcode140 locale/en-US/new-options.json:140: "message": "Whitelisted websites" On 2017/05/18 ...
3 months ago (2017-05-22 09:38:25 UTC) #8
Thomas Greiner
https://codereview.adblockplus.org/29411555/diff/29442607/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29442607/new-options.js#newcode773 new-options.js:773: getMessage("options_whitelist_duplicate"); Detail: This text doesn't appear to be used ...
2 months, 4 weeks ago (2017-05-26 11:10:40 UTC) #9
saroyanm
https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newcode988 new-options.js:988: collections.whitelist.addItem(whitelistItem); On 2017/05/26 11:10:40, Thomas Greiner wrote: > Removing ...
2 months, 3 weeks ago (2017-05-30 12:30:44 UTC) #10
saroyanm
New patch uploaded, ready for review. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29445572/new-options.html#newcode176 new-options.html:176: <form action=""> On ...
2 months, 3 weeks ago (2017-05-31 08:30:28 UTC) #11
saroyanm
Noticed a nit. https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newcode767 new-options.js:767: let exampleValue = getMessages("options_whitelist_placeholder_example", Nit: This ...
2 months, 1 week ago (2017-06-14 11:51:22 UTC) #12
Thomas Greiner
Only mentioned small issues so we should be good to go after that. https://codereview.adblockplus.org/29411555/diff/29452559/locale/en-US/new-options.json File ...
2 months, 1 week ago (2017-06-16 10:35:46 UTC) #13
saroyanm
https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newcode100 new-options.js:100: // Make sure that duplicated whitelist entries are always ...
2 months, 1 week ago (2017-06-16 11:13:54 UTC) #14
Thomas Greiner
On 2017/06/16 11:13:54, saroyanm wrote: > That's a very good question, I didn't know about ...
2 months, 1 week ago (2017-06-16 11:36:55 UTC) #15
saroyanm
On 2017/06/16 11:36:55, Thomas Greiner wrote: > On 2017/06/16 11:13:54, saroyanm wrote: > > That's ...
2 months, 1 week ago (2017-06-16 12:12:54 UTC) #16
Thomas Greiner
On 2017/06/16 12:12:54, saroyanm wrote: > I do agree with you, that the behavior should ...
2 months, 1 week ago (2017-06-16 12:22:43 UTC) #17
saroyanm
On 2017/06/16 12:12:54, saroyanm wrote: > On 2017/06/16 11:36:55, Thomas Greiner wrote: > > On ...
2 months, 1 week ago (2017-06-16 12:23:28 UTC) #18
saroyanm
https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newcode100 new-options.js:100: // Make sure that duplicated whitelist entries are always ...
2 months, 1 week ago (2017-06-16 13:05:12 UTC) #19
saroyanm
https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newcode777 new-options.js:777: addWhitelistedDomain(); On 2017/06/16 10:35:46, Thomas Greiner wrote: > This ...
2 months, 1 week ago (2017-06-16 15:53:45 UTC) #20
saroyanm
https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newcode767 new-options.js:767: let exampleValue = getMessages("options_whitelist_placeholder_example", On 2017/06/16 10:35:46, Thomas Greiner ...
2 months, 1 week ago (2017-06-16 16:43:08 UTC) #21
saroyanm
New patch is ready to be reviewed. https://codereview.adblockplus.org/29411555/diff/29452559/locale/en-US/new-options.json File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29452559/locale/en-US/new-options.json#newcode140 locale/en-US/new-options.json:140: "message": "You’ve ...
2 months, 1 week ago (2017-06-16 16:44:07 UTC) #22
Thomas Greiner
2 months, 1 week ago (2017-06-16 17:28:29 UTC) #23
LGTM, only noticed a typo

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

https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js#newc...
new-options.js:100: // Make sure that newly added entries are always appear on
top in
Typo: Remove "are".

https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js#newc...
new-options.js:763: document.body.addEventListener("click", onClick, false);
On 2017/06/16 16:43:08, saroyanm wrote:
> I think I've learned something today:
> Apparently hitting Enter on "whitelisting-textbox" element causes [implicit
> submission](https://www.w3.org/TR/html5/single-page.html#implicit-submission).
> I'm not sure how reliable is this(Theoretically it should be, as it's defined
in
> the specs), anyway this is the reason why I have removed:
> `
> E("whitelisting-textbox").addEventListener("keyup", (e) =>
> {
>   if (getKey(e) == "Enter")
>   ....
> }, false);
> `
> implementaiton.

I was aware of an implicit submission causing the "submit" event to be
dispatched on the form element but it was news to me that it also causes a
"click" event to be dispatched on the submit button.

Anyway, since this is defined in the standard and since you verified that it
works, we can make use of it.

Personally, I usually prefer making behavior explicit rather than relying on
implicit mechanisms but it's up to you to choose which way you prefer.

https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js#newc...
new-options.js:773: E("whitelisting-add-button").disabled = !e.target.value;
On 2017/06/16 16:43:08, saroyanm wrote:
> Not sure if this also needs to be moved into `onKeyUp()` function.
> 
> Not sure if Element specific behavior belongs to `onKeyUp()` function.

I'd say either way is fine so let's keep it as is.
Sign in to reply to this message.

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