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

Show an adblock admonition in the dev console #3894

Merged
merged 3 commits into from
Apr 17, 2018

Conversation

davidfischer
Copy link
Contributor

This PR shows an adblock admonition in the browser developer console. While I don't think this will convert many hearts and minds, I believe it is a good first step.

screen shot 2018-04-02 at 10 24 10 pm

@davidfischer
Copy link
Contributor Author

To test this locally, change api_host in user_builds/<project>/rtd-builds/latest/_static/readthedocs-data.js to https://readthedocs.org and load those docs.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems simple enough. Doubt it will do much, but seems worth testing out.

console.error('Error loading Read the Docs promo');

if (xhr && xhr.status === 404 && rtd.api_host === 'https://readthedocs.org') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel like we probably need more of a check here. Is there an error thrown when it's blocked? In chrome I see "Blocked by the client" -- is there a specific error we should be looking for that is similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From jQuery's perspective, it is just a 404.

@davidfischer
Copy link
Contributor Author

I don't think this will do anything with respect to getting people to whitelist us. However, it illustrates how to detect an ad blocker and simply logs to the console (which might alert us to any false positives) before we take any further action.

console.log(' - only show advertisements of interest to developers');
console.log('You can read more about our approach to advertising here: https://docs.readthedocs.io/en/latest/ethical-advertising.html');

console.log('%cPlease whitelist *.readthedocs.io on your adblocker!', 'font-size: 2em');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to say *.readthedocs.org maybe?

Look at this

captura de pantalla_2018-04-04_12-07-37

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it comes from EasyList

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good catch. I need to review this. We may need both...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a larger problem than I thought.

Whitelisting *.readthedocs.io is all that is necessary (on AdblockPlus at least) to allow Read the Docs ads on all of our sites hosted on a subdomain. However, ads will continue to be blocked on any custom domains where we host ads. Whitelisting readthedocs.org does not change this. This makes simple whitelisting impossible across all ~10,000 domains where we host ads.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing their filter syntax, we need the following two filters:

  • @@||readthedocs.org/* (exception for making an ad request)
  • @@readthedocs.io^$document (exception for hiding elements)

@davidfischer
Copy link
Contributor Author

I noted the specific commands needed to whitelist across our custom domains.

@agjohnson agjohnson added this to the 2.4 milestone Apr 17, 2018
@davidfischer davidfischer merged commit 5c9fd18 into readthedocs:master Apr 17, 2018
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.

4 participants