Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ad blocking #98

Merged
merged 6 commits into from
Feb 2, 2022
Merged

Support ad blocking #98

merged 6 commits into from
Feb 2, 2022

Conversation

jopemachine
Copy link
Contributor

Fixes #7.

I think we could use @cliqz/adblocker-puppeteer package to support ad blocking.

Notes

  • This draft PR does not include the option representing whether to use the ad-blocker, related option, type, tests and documentations yet.

  • If I'm on the right way, I'd appreciate it if you could let me know. Then I will try to do the next works.

@sindresorhus
Copy link
Owner

Yes, this looks like the right path.

What do you think about making ad-blocking opt-out instead of opt-in?

@jopemachine
Copy link
Contributor Author

Yes, this looks like the right path.

What do you think about making ad-blocking opt-out instead of opt-in?

In my opinion, opt-out would be better. I think there are likely to be more users who do not need ads in their screenshots than those who do not.

@sindresorhus
Copy link
Owner

Opt-out it is then.

index.d.ts Outdated Show resolved Hide resolved
@jopemachine
Copy link
Contributor Author

@sindresorhus I've been thinking about the unit testing of this flag.
Do I need to compare generated screenshot having an ad with the one without the ad? (It seems that using resemble.js is one way to do this)
Would it be there some better methods to test blockAds option?

@sindresorhus
Copy link
Owner

I think manually verifying it works is enough. A unit test will fail at some point when ads or ad blocker changes.

@jopemachine jopemachine marked this pull request as ready for review January 31, 2022 12:11
@jopemachine
Copy link
Contributor Author

I think manually verifying it works is enough. A unit test will fail at some point when ads or ad blocker changes.

Ok, I just made this Draft PR ready. Please let me know if there are other things for me to do :)

@sindresorhus
Copy link
Owner

Doesn't look like you updated the TS types.

@sindresorhus sindresorhus changed the title Support Ad blocking Support ad blocking Feb 2, 2022
@sindresorhus sindresorhus merged commit 10e42d9 into sindresorhus:main Feb 2, 2022
@sindresorhus
Copy link
Owner

Thanks :)

Would you be able to add a --no-block-ads flag to the CLI?

@jopemachine
Copy link
Contributor Author

Thanks :)

Would you be able to add a --no-block-ads flag to the CLI?

Sure, I'll make a PR to CLI module as soon as capture-website's next version is released :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ad blocking
2 participants