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.

Sign up for GitHub

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

Jump to bottom

amp-ad type=taboola #1515

Merged
merged 1 commit into from
Feb 2, 2016
Merged

amp-ad type=taboola #1515

merged 1 commit into from
Feb 2, 2016

Conversation

nitzanvolman
Copy link
Contributor

Provides support for amp-ad type=taboola

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@nitzanvolman nitzanvolman mentioned this pull request Jan 21, 2016
Closed
@nitzanvolman nitzanvolman changed the title With taboola ad type amp-ad type=taboola Jan 21, 2016
@cramforce
Copy link
Member

Sorry, I did not realize there were overlapping PRs. Made comments on #1518. I believe we should land only this one.

jridgewell
jridgewell reviewed Jan 22, 2016
View reviewed changes
ads/taboola.js
};

validateData(data, ['publisher', 'placement', 'mode']);
validateExactlyOne(data, ['article', 'video', 'photo', 'search', 'category', 'homepage', 'others']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move the above lines outside the taboola function? I'm pretty sure they're stateless, which will give marginal benefits when using multiple ads.

@googlebot
Copy link

CLAs look good, thanks!

@nitzanvolman
Copy link
Contributor Author

@cramforce handled your comments form #1518 squashed and rebased.
@dvoytenko @Effie - we are getting a validation error on resizable attribute on amp-embed and amp-ad. I believe this was valid previously. Is the resizable attribute required for the resizing functionality (the ability for the ad to signal a different size?)
I will remove the use of it now, so we get a green build.

@dvoytenko
Copy link
Contributor

@nitzanvolman if it directly concerns amp-ad - no resizable attribute is allowed or needed, I believe.

@nitzanvolman
Copy link
Contributor Author

thanks @dvoytenko - so just to clarify, amp will still respond to resize messages from and amp-ad, just the resize attribute is redundant? is this correct?

@dvoytenko
Copy link
Contributor

@nitzanvolman I believe this is correct. I'll ask @sriramkrish85 to chime in.

@nitzanvolman
Copy link
Contributor Author

Awesome, in that case i think we are ready @dvoytenko - unless anyone sees any issues we missed.
(@Effie)

@camelburrito
Copy link
Contributor

@dvoytenko @nitzanvolman is correct - amp will still respond to resize messages from and amp-ad and does not need a resizable attribute for that.

#1596

cramforce
cramforce reviewed Feb 2, 2016
View reviewed changes
3p/integration.js
@@ -35,12 +35,14 @@ import {twitter} from './twitter';
import {register, run} from '../src/3p';
import {parseUrl} from '../src/url';
import {assert} from '../src/asserts';
import {taboola} from '../ads/taboola';
Copy link
Member

Choose a reason for hiding this comment

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

I added a mechanism to whitelist which embed types are allowed to use amp-embed. After rebasing add yourself to AMP_EMBED_ALLOWED in this file.

@cramforce cramforce self-assigned this Feb 2, 2016
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@cramforce
Copy link
Member

Re-running tests as that might have been a flake.

Please squash commits and we can merge this. Thanks!

@googlebot
Copy link

CLAs look good, thanks!

@nitzanvolman
Copy link
Contributor Author

Rebased, squashed.

cramforce added a commit that referenced this pull request Feb 2, 2016
@cramforce
Merge pull request #1515 from taboola/With-taboola-ad-type
2965970 
amp-ad type=taboola
@cramforce cramforce merged commit 2965970 into ampproject:master Feb 2, 2016
@cramforce
Copy link
Member

LGTM and merged. Thanks!

@henel677 henel677 mentioned this pull request Mar 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cramforce cramforce

Labels
WG: monetization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nitzanvolman @googlebot @cramforce @dvoytenko @camelburrito @jridgewell