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

Issue 29851644: Issue 6848 - Add searchSelector parameter to hide-if-contains snippet (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Manish Jethani
Modified:
2 months, 1 week ago
Reviewers:
hub
CC:
kzar
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 #

Total comments: 3

Patch Set 2 : Use Element.closest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -18 lines) Patch
M lib/content/snippets.js View 1 3 chunks +20 lines, -18 lines 0 comments Download

Messages

Total messages: 5
Manish Jethani
2 months, 1 week ago (2018-08-09 15:40:23 UTC) #1
Manish Jethani
Patch Set 1
2 months, 1 week ago (2018-08-09 16:04:27 UTC) #2
hub
https://codereview.adblockplus.org/29851644/diff/29851645/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29851644/diff/29851645/lib/content/snippets.js#newcode136 lib/content/snippets.js:136: if (element.matches(selector)) Wouldn't https://developer.mozilla.org/en-US/docs/Web/API/Element/closest work here?
2 months, 1 week ago (2018-08-10 12:30:41 UTC) #3
Manish Jethani
Patch Set 2: Use Element.closest https://codereview.adblockplus.org/29851644/diff/29851645/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29851644/diff/29851645/lib/content/snippets.js#newcode136 lib/content/snippets.js:136: if (element.matches(selector)) On 2018/08/10 ...
2 months, 1 week ago (2018-08-10 14:57:02 UTC) #4
hub
2 months, 1 week ago (2018-08-10 19:54:57 UTC) #5
LGTM

https://codereview.adblockplus.org/29851644/diff/29851645/lib/content/snippet...
File lib/content/snippets.js (right):

https://codereview.adblockplus.org/29851644/diff/29851645/lib/content/snippet...
lib/content/snippets.js:136: if (element.matches(selector))
On 2018/08/10 14:57:02, Manish Jethani wrote:
> On 2018/08/10 12:30:41, hub wrote:
> > Wouldn't https://developer.mozilla.org/en-US/docs/Web/API/Element/closest
work
> > here?
> 
> Ah, you're right. Element.closest is perfect.
> 
> It's only supported from Edge 15 onwards but I'm not sure we have to worry
about
> that.

We support EdgeHTML 16 and up if I read things correctly.
Sign in to reply to this message.

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