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

✨ Add tracdelight AMP ad integration #23124

Merged
merged 4 commits into from Oct 8, 2019

Conversation

thekorn
Copy link
Contributor

@thekorn thekorn commented Jul 1, 2019

The pull request contains implementation for the tracdelight ad-widgets for AMP Ads.
Reference to our closed PRs are #13956 #22550

@mrjoro
Copy link
Member

mrjoro commented Aug 8, 2019

Sorry for the delay in reviewing this @thekorn!

/cc @ampproject/wg-ads

@mrjoro mrjoro requested a review from lannka August 8, 2019 18:54
@lannka lannka requested review from calebcordry and removed request for lannka August 9, 2019 16:56
@calebcordry
Copy link
Member

@thekorn Thanks for contributing. When I try to run you example locally it doesn't seem to work. Looks like the request to https://api.tracdelight.com/v1/widgets/false/products?accesskey=false might not behave as expected. Would it be possible to return an example ad here? It makes it easier for us to have a working example so that we can ensure we don't break anything etc.

@calebcordry
Copy link
Member

Also you might want to rebase since this pr has been out for a while (our fault). I had some weird errors before rebasing. Let me know if you need any assistance.

@thekorn
Copy link
Contributor Author

thekorn commented Aug 26, 2019

thanks @calebcordry for the review, we will look into it and come back to you

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@thekorn
Copy link
Contributor Author

thekorn commented Aug 30, 2019

@calebcordry I've one question: I just pushed one change, and we got flagged cla: no again - altough I'm still the only author in the changeset. What is the reason for this?

@calebcordry
Copy link
Member

@thekorn I see that the CLA was approved at one point, but when I look at our internal system it says your username is not registered. What do you see when you follow this link?

@thekorn
Copy link
Contributor Author

thekorn commented Sep 6, 2019

@calebcordry this is what I get on this page:
image

@calebcordry
Copy link
Member

@thekorn in that screenshot I see your @gmail account, but the commits are signed from your @tracdelight account. I think that may be causing the issue. Could you confirm that your tracdelight email is also covered under the corporate CLA? Alternatively you could try changing the author of your commits to be your gmail address and see if that works.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@calebcordry
Copy link
Member

Awesome, looks like you got the CLA all sorted out. I still don't see a working example, did this get lost in the CLA work?

Also, I don't see renderStartImplemented being called. Can you confirm that this is called in your JS? If not you might want to remove it from your config. We suggest implementing it, but if you have the flag as true without implementing, performance is actually worse. Let me know if you need any help. Thanks!

@@ -0,0 +1,31 @@
/**
* Copyright 2018 The AMP HTML Authors. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

nit: 2019

Copy link
Member

Choose a reason for hiding this comment

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

Can you change this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@calebcordry
Copy link
Member

calebcordry commented Sep 11, 2019

image

Image of broken ad in case you wanted to see.

@thekorn
Copy link
Contributor Author

thekorn commented Sep 13, 2019

@calebcordry okay, thanks for your feedback - unfortunately the sample widget was not active anymore - I changed the widget id and now it's working properly.

2019-09-13 at 16 33

@thekorn
Copy link
Contributor Author

thekorn commented Oct 7, 2019

Hi @calebcordry and @mrjoro
just one question, as a kind of follow up, is there anything needed here from my side? anything I missed?
and what are the next steps here?
thanks for your help

@@ -0,0 +1,44 @@
<!---
Copyright 2018 The AMP HTML Authors. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

nit: 2019

@calebcordry
Copy link
Member

Sorry fo the delay. Just the year nits, and this should be ready.

@calebcordry
Copy link
Member

Thanks, looks like a travis test flake. I just restarted, I will try to keep an eye on it, but let me know if you see it pass.

@calebcordry
Copy link
Member

Passed :) thanks for contributing!

@calebcordry calebcordry merged commit 7991473 into ampproject:master Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants