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

Sharethrough render-start support added #5531

Merged
merged 2 commits into from Oct 19, 2016

Conversation

peterkinmond
Copy link
Contributor

Hi, we've added the "renderStart" and "noContentAvailable" methods (refer to #5234).
Could you please review?

Thanks!

@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.

@jridgewell
Copy link
Contributor

/to @lannka

@peterkinmond
Copy link
Contributor Author

peterkinmond commented Oct 11, 2016

We signed the Google CLA on behalf of my company Sharethrough and it should be under their name.

@lannka
Copy link
Contributor

lannka commented Oct 11, 2016

CLA: Sharethrough

Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

@peterkinmond I don't see ad loaded in ads.amp.html. It timed out and fallback to "no ad". Did you try to correctly call renderStart in your SDK?

Please take a look at the developer guidelines and try to make it work locally.

@lannka
Copy link
Contributor

lannka commented Oct 12, 2016

CLA robot didn't recognize. Maybe try to mention your corp again?

@jridgewell
Copy link
Contributor

@peterkinmond: When using a Corp CLA, you have to author commits using a corp email address (@sharethrough.com).

@lannka
Copy link
Contributor

lannka commented Oct 13, 2016

When using a Corp CLA, you have to author commits using a corp email address (@sharethrough.com).

@jridgewell oh that's the trick? I have no idea what happened in signing corp CLA, does Google collect the corp email domain?

@jridgewell
Copy link
Contributor

I think so.

@peterkinmond
Copy link
Contributor Author

@jridgewell Is needing a corp email address a new rule? I'm a member of the Sharethrough org on Github and last time we added to AMP project it wasn't with a corp email address.

@peterkinmond
Copy link
Contributor Author

@lannka Thanks for the quick response in testing out the PR. I see a Sharethrough ad loading consistently on http://localhost:8000/examples/ads.amp.max.html when running it locally on my machine. I looked through the developer guidelines you linked to and everything seems to be correct. Is there something else I can do to confirm it's working correctly?

screen shot 2016-10-13 at 3 25 25 pm

@lannka
Copy link
Contributor

lannka commented Oct 14, 2016

Would that be because of any ad targeting so we fall into different loading path?

screen shot 2016-10-13 at 5 20 02 pm

@peterkinmond
Copy link
Contributor Author

@lannka Ok we've made an update on our side which should now fix it and fire the events correctly. Can you try again?

@lannka
Copy link
Contributor

lannka commented Oct 18, 2016

It works now.

@cramforce could you please force merge. We have CLA problem again.

@lannka lannka added LGTM and removed NEEDS REVIEW labels Oct 18, 2016
@cramforce
Copy link
Member

Happy to :)

@cramforce cramforce merged commit 9ece51c into ampproject:master Oct 19, 2016
@peterkinmond
Copy link
Contributor Author

Thanks!

@peterkinmond peterkinmond deleted the sharethroughRenderStart branch October 19, 2016 00:05
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants