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

Community only ads for more themes #5973

Merged
merged 1 commit into from Jul 23, 2019

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Jul 21, 2019

  • Request community only ads on themes where we don't support regular ads
  • This should not be merged until the relevant server side changes are merge in readthedocs/readthedocs-ext#247
@davidfischer davidfischer requested a review from Jul 21, 2019
@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Jul 21, 2019

Does this also targets the error here -- http://2.python-requests.org/en/v2.2.1/search/?q=get ?

Copy link
Member

@humitos humitos left a comment

The code looks good.

I'm not sure to understand what it means that the theme does not support regular Ads but it supports Community Ads.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Jul 22, 2019

Does this also targets the error here

The problems with search are unrelated to this change.

I'm not sure to understand what it means that the theme does not support regular Ads but it supports Community Ads.

As part of the rollout of the new fixed footer ads, we stated:

At first, we will show only community and house ads on these custom themes.

this.api_host !== 'https://readthedocs.com' &&
this.theme_supports_promo());
this.api_host === 'https://readthedocs.org' ||
this.api_host === 'http://127.0.0.1:8000'
Copy link
Member

@ericholscher ericholscher Jul 22, 2019

Should this also check for localhost? Also feels a bit hacky, but I don't know a better way.

Copy link
Contributor Author

@davidfischer davidfischer Jul 22, 2019

The default in settings/base.py is 127.0.0.1:8000. It is a bit hacky but the above change covers the defaults.

@ericholscher ericholscher merged commit 9a1dd52 into master Jul 23, 2019
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the davidfischer/api-flag-community-only branch Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants