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

Use sustainability api for advertising #3747

Merged
merged 8 commits into from Mar 13, 2018

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Mar 7, 2018

This updates our front-end advertising code to use the new sustainability API for advertising decisions.

  • Pushes ad HTML generation to the server as much as possible
  • No longer is the ad decision part of the footer insertion (they both happen async)

NOTE: This is built off of #3672. If this is merged, that one can be closed.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 7, 2018

Hmm. This is hard to review because of the file rename. Is there a good way to see the logic changes as a diff?

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 7, 2018

Looking at it, I guess it's a totally different pattern, so ignore me :)

Loading

Copy link
Member

@ericholscher ericholscher left a comment

This looks like a really nice pattern to follow. It should also allow us to trivially document an explicit div_id where users would want us to inject things, in order to actually build that logic into the themes and other bits going forward.

A few small nits, but overall I really like this pattern. Would be good to get @agjohnson to take a look as well, since he has more context around the JS & CSS bits.

Loading

post_data.placements = get_placements(rtd);
post_data.project = rtd.project;

if (typeof URL !== 'undefined' && typeof URLSearchParams !== 'undefined') {
Copy link
Member

@ericholscher ericholscher Mar 7, 2018

Choose a reason for hiding this comment

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

Where do these variables come from? They look like new JS things that will not work on older browsers? https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams

Loading

Copy link
Contributor Author

@davidfischer davidfischer Mar 7, 2018

Choose a reason for hiding this comment

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

That is correct. They will not. Hence the typeof checking. Old browsers will not be able to force a specific promo or campaign. However, with the typeof checking they will not trigger errors.

Loading

Copy link
Member

@ericholscher ericholscher Mar 7, 2018

Choose a reason for hiding this comment

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

Gotcha. I do worry about using newer browser features for something whose primary purpose is linking to others, because it might break on their browser. We can change it if it becomes an issue though.

Loading

Copy link
Contributor Author

@davidfischer davidfischer Mar 7, 2018

Choose a reason for hiding this comment

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

Unfortunately reading GET params without these builtins or adding a purpose built library becomes somewhat non-trivial.

Loading

Copy link
Contributor Author

@davidfischer davidfischer Mar 7, 2018

Choose a reason for hiding this comment

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

URLSearchParams is supported everywhere but older IE/Edge versions. This is ~3% of our traffic overall. I did verify this code path on IE11 to make sure there isn't an issue.

https://caniuse.com/#feat=urlsearchparams

Loading

if (typeof URL !== 'undefined' && typeof URLSearchParams !== 'undefined') {
// Force a specific promo to be displayed
params = new URL(window.location).searchParams;
force = params.get('promo');
Copy link
Member

@ericholscher ericholscher Mar 7, 2018

Choose a reason for hiding this comment

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

Could use a better variable name here, this reads like a boolean. Perhaps explicit_promo or forced_promo

Loading

Copy link
Contributor Author

@davidfischer davidfischer Mar 7, 2018

Choose a reason for hiding this comment

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

Sounds good.

Loading

}

// Force a promo from a specific campaign
force = params.get('campaign');
Copy link
Member

@ericholscher ericholscher Mar 7, 2018

Choose a reason for hiding this comment

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

Similar, shouldn't repeat the variable name, should use something explicit.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Mar 7, 2018

I made the changes based on initial feedback.

Loading

@ericholscher ericholscher merged commit 3a67d11 into readthedocs:master Mar 13, 2018
1 check passed
Loading
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

2 participants