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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by Thomas Greiner
Modified:
3 years, 10 months 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
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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: > ...
3 years, 10 months 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 ...
3 years, 10 months 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: ...
3 years, 10 months 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 ...
3 years, 10 months 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, ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months ago (2015-07-14 12:45:09 UTC) #12
Sebastian Noack
LGTM
3 years, 10 months 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 ...
3 years, 10 months ago (2015-07-16 11:42:55 UTC) #14
Sebastian Noack
Still LGTM
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months ago (2015-07-16 12:36:51 UTC) #17
saroyanm
3 years, 10 months 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