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

Issue 29321417: Issue 2357 - Added "predefined list" dialog to options page (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years ago by Thomas Greiner
Modified:
4 years ago
CC:
Felix Dahlke
Visibility:
Public.

Description

To implement that dialog I decided to also make the following changes: - Aggregate click events by using single event listener to centralize handling of those events (removed unused IDs as a result of that) - Aggregate styles which are responsible for showing a specific dialog into a single selector

Patch Set 1 #

Total comments: 14

Patch Set 2 : #

Patch Set 3 : Replaced dataset with has/get/set/removeAttribute #

Total comments: 10

Patch Set 4 : #

Total comments: 7

Patch Set 5 : #

Patch Set 6 : post-review: rebased #

Patch Set 7 : Post-review: Merged #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+140 lines, -122 lines) Patch
M locale/en-US/options.json View 1 2 3 4 5 1 chunk +13 lines, -5 lines 0 comments Download
M options.html View 1 2 3 4 5 6 7 chunks +34 lines, -22 lines 0 comments Download
M options.js View 1 2 3 4 5 6 9 chunks +78 lines, -74 lines 3 comments Download
M skin/options.css View 1 2 3 4 5 3 chunks +15 lines, -21 lines 0 comments Download

Messages

Total messages: 18
Thomas Greiner
4 years ago (2015-07-03 16:09:01 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode324 options.js:324: if (element === document.body) Nit: We don't use the ...
4 years ago (2015-07-09 12:24:19 UTC) #2
Thomas Greiner
Thanks for helping out with the reviews. https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode324 options.js:324: if (element ...
4 years ago (2015-07-09 13:55:07 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode324 options.js:324: if (element === document.body) On 2015/07/09 13:55:06, Thomas Greiner ...
4 years ago (2015-07-09 15:16:57 UTC) #4
saroyanm
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333 options.js:333: switch (element.dataset.action) On 2015/07/09 15:16:56, Sebastian Noack wrote: > ...
4 years ago (2015-07-09 17:14:19 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333 options.js:333: switch (element.dataset.action) Well, by relying on flexbox, the new ...
4 years ago (2015-07-10 07:58:19 UTC) #6
Thomas Greiner
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode330 options.js:330: element = element.parentNode; On 2015/07/09 15:16:56, Sebastian Noack wrote: ...
4 years ago (2015-07-10 14:19:15 UTC) #7
saroyanm
https://codereview.adblockplus.org/29321417/diff/29321580/locale/en-US/options.json File locale/en-US/options.json (right): https://codereview.adblockplus.org/29321417/diff/29321580/locale/en-US/options.json#newcode168 locale/en-US/options.json:168: "message": "Yes, use this blocking list" In the mockup ...
4 years ago (2015-07-13 16:20:07 UTC) #8
Thomas Greiner
https://codereview.adblockplus.org/29321417/diff/29321580/locale/en-US/options.json File locale/en-US/options.json (right): https://codereview.adblockplus.org/29321417/diff/29321580/locale/en-US/options.json#newcode168 locale/en-US/options.json:168: "message": "Yes, use this blocking list" On 2015/07/13 16:20:07, ...
4 years ago (2015-07-13 18:37:15 UTC) #9
saroyanm
Just small consistency related changes. https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/options.json File locale/en-US/options.json (right): https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/options.json#newcode167 locale/en-US/options.json:167: "description": "Confirming to add ...
4 years ago (2015-07-14 11:03:38 UTC) #10
Thomas Greiner
https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/options.json File locale/en-US/options.json (right): https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/options.json#newcode167 locale/en-US/options.json:167: "description": "Confirming to add a predefined subscription", On 2015/07/14 ...
4 years ago (2015-07-14 11:37:34 UTC) #11
saroyanm
On 2015/07/14 11:37:34, Thomas Greiner wrote: > https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/options.json > File locale/en-US/options.json (right): > > https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/options.json#newcode167 ...
4 years ago (2015-07-14 12:45:09 UTC) #12
Sebastian Noack
LGTM
4 years ago (2015-07-15 15:01:14 UTC) #13
Thomas Greiner
After #2376 has landed it made sense to consider the newly added click listener in ...
4 years ago (2015-07-16 11:42:55 UTC) #14
Sebastian Noack
Still LGTM
4 years ago (2015-07-16 11:44:04 UTC) #15
saroyanm
LGTM https://codereview.adblockplus.org/29321417/diff/29322498/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29322498/options.js#newcode364 options.js:364: E("custom-filters").classList.remove("mode-edit"); Strange, classList according to MDN is supported ...
4 years ago (2015-07-16 12:32:27 UTC) #16
Sebastian Noack
https://codereview.adblockplus.org/29321417/diff/29322498/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29322498/options.js#newcode364 options.js:364: E("custom-filters").classList.remove("mode-edit"); On 2015/07/16 12:32:27, saroyanm wrote: > Strange, classList ...
4 years ago (2015-07-16 12:36:51 UTC) #17
saroyanm
4 years ago (2015-07-16 12:41:02 UTC) #18
https://codereview.adblockplus.org/29321417/diff/29322498/options.js
File options.js (right):

https://codereview.adblockplus.org/29321417/diff/29322498/options.js#newcode364
options.js:364: E("custom-filters").classList.remove("mode-edit");
On 2015/07/16 12:36:50, Sebastian Noack wrote:
> On 2015/07/16 12:32:27, saroyanm wrote:
> > Strange, classList according to MDN is supported since safari 5.1:
> >
>
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_co...
> > 
> > But according to caniuse:
> > http://caniuse.com/#search=classList
> > It's supported since safari 7
> > 
> > But I do think it's mistake in caniuse because they are referencing to MDN
and
> > some other articles where nothing mentioned about classList being supported
> > since safari 7. 
> > So we should be good to go with classList.
> 
> As I already told you in a different comment, it's only classList.toggle()
with
> second argument that doesn't work in older Safari versions. classList as we
use
> it here should be fine.

Ahh right your reference to caniuse went out of my radar.
Sign in to reply to this message.

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