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

Issue 5838948538515456: Issue 370 - Make "Block element" hide elements for added filters (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by Sebastian Noack
Modified:
4 years, 11 months ago
Reviewers:
Wladimir Palant, kzar
CC:
Thomas Greiner
Visibility:
Public.

Description

Issue 370 - Make "Block element" hide elements for added filters

Patch Set 1 : #

Total comments: 7

Patch Set 2 : Added comments #

Total comments: 3

Patch Set 3 : Restored failsafe code #

Patch Set 4 : Fixed failsafe code #

Total comments: 2

Patch Set 5 : Improved comment #

Patch Set 6 : Replace existing rules #

Total comments: 5

Patch Set 7 : Addressed comments and simplified loop #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -42 lines) Patch
M include.postload.js View 1 2 3 4 5 1 chunk +9 lines, -4 lines 0 comments Download
M include.preload.js View 1 2 3 4 5 6 4 chunks +50 lines, -38 lines 0 comments Download

Messages

Total messages: 19
Sebastian Noack
4 years, 11 months ago (2015-03-04 10:10:38 UTC) #1
kzar
I think this is a cool idea but I think with blocking rules it will ...
4 years, 11 months ago (2015-03-04 11:04:02 UTC) #2
kzar
Maybe we should vanish the element user right clicked on if there are no element ...
4 years, 11 months ago (2015-03-04 11:13:21 UTC) #3
kzar
(Or maybe if there are blocking rules generated we should somehow notify the user that ...
4 years, 11 months ago (2015-03-04 11:16:43 UTC) #4
Sebastian Noack
Blocking rules are still considered, though in a limited way. After the filters were added, ...
4 years, 11 months ago (2015-03-04 11:34:44 UTC) #5
Sebastian Noack
http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/include.postload.js#newcode726 include.postload.js:726: var selectors = msg.selectors; On 2015/03/04 11:04:02, kzar wrote: ...
4 years, 11 months ago (2015-03-04 11:34:52 UTC) #6
kzar
Oh cool, glad to hear it. In that case I think your approach is fine, ...
4 years, 11 months ago (2015-03-04 13:09:03 UTC) #7
Sebastian Noack
Other changes due to rebase http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/include.postload.js#newcode726 include.postload.js:726: var selectors = msg.selectors; ...
4 years, 11 months ago (2015-03-04 14:02:01 UTC) #8
kzar
LGTM http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/include.postload.js#newcode726 include.postload.js:726: var selectors = msg.selectors; On 2015/03/04 14:02:01, Sebastian ...
4 years, 11 months ago (2015-03-04 14:04:14 UTC) #9
Wladimir Palant
Other than the issue below, the changes look fine to me. http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/include.preload.js File include.preload.js (left): ...
4 years, 11 months ago (2015-03-04 14:45:54 UTC) #10
Sebastian Noack
http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/include.preload.js File include.preload.js (left): http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/include.preload.js#oldcode226 include.preload.js:226: setTimeout(setRules, 0); On 2015/03/04 14:45:54, Wladimir Palant wrote: > ...
4 years, 11 months ago (2015-03-04 15:10:22 UTC) #11
Sebastian Noack
http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/include.preload.js File include.preload.js (left): http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/include.preload.js#oldcode226 include.preload.js:226: setTimeout(setRules, 0); On 2015/03/04 15:10:22, Sebastian Noack wrote: > ...
4 years, 11 months ago (2015-03-04 18:55:10 UTC) #12
Wladimir Palant
http://codereview.adblockplus.org/5838948538515456/diff/5709068098338816/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5709068098338816/include.preload.js#newcode214 include.preload.js:214: // property stays null, after adding the <style> element ...
4 years, 11 months ago (2015-03-04 19:06:15 UTC) #13
Sebastian Noack
http://codereview.adblockplus.org/5838948538515456/diff/5709068098338816/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5709068098338816/include.preload.js#newcode214 include.preload.js:214: // property stays null, after adding the <style> element ...
4 years, 11 months ago (2015-03-04 19:34:19 UTC) #14
Wladimir Palant
LGTM
4 years, 11 months ago (2015-03-04 19:35:29 UTC) #15
Sebastian Noack
I've uploaded a new patch set, that instead of applying the added element hiding filters, ...
4 years, 11 months ago (2015-03-04 21:15:40 UTC) #16
Wladimir Palant
Actually, I also like that variant better - it's just less of a hack than ...
4 years, 11 months ago (2015-03-04 21:26:14 UTC) #17
Sebastian Noack
http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/block.js File block.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/block.js#newcode57 block.js:57: closeDialog(false); On 2015/03/04 21:26:14, Wladimir Palant wrote: > Nit: ...
4 years, 11 months ago (2015-03-04 21:36:23 UTC) #18
Wladimir Palant
4 years, 11 months ago (2015-03-04 21:51:49 UTC) #19
LGTM
Sign in to reply to this message.

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