Skip to content

Add support for "renderStart" and "noContentAvailable" for adup-tech ads#7152

Merged
lannka merged 1 commit intoampproject:masterfrom
adup-archive:update_adup_tech
Feb 2, 2017
Merged

Add support for "renderStart" and "noContentAvailable" for adup-tech ads#7152
lannka merged 1 commit intoampproject:masterfrom
adup-archive:update_adup_tech

Conversation

@SteffenAnders
Copy link
Copy Markdown
Contributor

@SteffenAnders SteffenAnders commented Jan 23, 2017

@googlebot
Copy link
Copy Markdown

googlebot commented Jan 23, 2017

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.

@SteffenAnders
Copy link
Copy Markdown
Contributor Author

SteffenAnders commented Jan 23, 2017

I signed it!

Company: Ad Up Technology AG
Google Group: ad-up-technology-ag@googlegroups.com
E-Mail: steffen.anders@adup-tech.com

see: #2779

@SteffenAnders SteffenAnders force-pushed the update_adup_tech branch 3 times, most recently from 247a253 to 38ed84c Compare January 23, 2017 12:10
@SteffenAnders SteffenAnders mentioned this pull request Jan 23, 2017
Closed
65 tasks
@jridgewell jridgewell requested a review from lannka January 23, 2017 23:03
@jridgewell
Copy link
Copy Markdown
Contributor

jridgewell commented Jan 23, 2017

/to @lannka

lannka
lannka approved these changes Jan 25, 2017
View reviewed changes
Copy link
Copy Markdown
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.

Awesome!
Minor comments

ads/aduptech.js

// ads callback => render start
data.onAds = () => {
global.context.renderStart();
Copy link
Copy Markdown
Contributor

@lannka lannka Jan 25, 2017

Choose a reason for hiding this comment

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

would this work ?

data.onAds = global.context.renderStart

Copy link
Copy Markdown
Contributor Author

@SteffenAnders SteffenAnders Jan 25, 2017

Choose a reason for hiding this comment

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

no (see inline doc)

ads/aduptech.js Outdated

// no ads callback => noContentAvailable
data.onNoAds = () => {
global.context.noContentAvailable();
Copy link
Copy Markdown
Contributor

@lannka lannka Jan 25, 2017

Choose a reason for hiding this comment

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

ditto

Copy link
Copy Markdown
Contributor Author

@SteffenAnders SteffenAnders Jan 25, 2017

Choose a reason for hiding this comment

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

yes, fixed

@SteffenAnders
Copy link
Copy Markdown
Contributor Author

SteffenAnders commented Jan 25, 2017

rebased to master

@SteffenAnders
Copy link
Copy Markdown
Contributor Author

SteffenAnders commented Jan 27, 2017

Rebased to master again.
Is there a problem with the CLA? :(

@SteffenAnders
Copy link
Copy Markdown
Contributor Author

SteffenAnders commented Jan 31, 2017

Rebased to master

@jridgewell
it seems that the cla/google status is not passing properly. Did i make any mistakes?

I signed it!

Company: Ad Up Technology AG
Google Group: ad-up-technology-ag@googlegroups.com
E-Mail: steffen.anders@adup-tech.com

@jridgewell
Copy link
Copy Markdown
Contributor

jridgewell commented Jan 31, 2017

@lannka can manually verify your CLA.

@lannka lannka self-assigned this Jan 31, 2017
@lannka
Copy link
Copy Markdown
Contributor

lannka commented Jan 31, 2017

@SteffenAnders I couldn't find Ad Up in our CLA list. Did you follow the bot's guide to sign the CLA?

@SteffenAnders
Copy link
Copy Markdown
Contributor Author

SteffenAnders commented Feb 1, 2017

I signed it!

@SteffenAnders
Copy link
Copy Markdown
Contributor Author

SteffenAnders commented Feb 1, 2017 •
edited
Loading

Rebased to master

@lannka
I am a member with my e-mail-address "steffen.anders@adup-tech.com" of my coorperations googlegroup "ad-up-technology-ag@googlegroups.com" which signed the CLA. I also used the e-mail-address "steffen.anders@adup-tech.com" and my github username "SteffenAnders" as author in the git commit.

PS: My coorperation signed the CLA again, maybe it works now. :)

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Feb 1, 2017

Can you confirm the corp name contains string of "Ad up"? I don't see any on the list.
+@erwinmombay to back me up

@SteffenAnders
Copy link
Copy Markdown
Contributor Author

SteffenAnders commented Feb 2, 2017 •
edited
Loading

@lannka The corp name is "Ad Up Technology AG". What list do you mean? :)

@googlebot
Copy link
Copy Markdown

googlebot commented Feb 2, 2017

CLAs look good, thanks!

@SteffenAnders
Copy link
Copy Markdown
Contributor Author

SteffenAnders commented Feb 2, 2017

  • Rebased to master
  • Signed CLA additionaly for myself

@erwinmombay
Copy link
Copy Markdown
Member

erwinmombay commented Feb 2, 2017

looks like this is green

@lannka lannka merged commit c7cb59f into ampproject:master Feb 2, 2017
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Feb 14, 2017
@SteffenAnders @torch2424
Add support for "renderStart" and "noContentAvailable" for adup-tech …
d9a0a64 
…ads (ampproject#7152)
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
@SteffenAnders @mrjoro
Add support for "renderStart" and "noContentAvailable" for adup-tech …
4552de2 
…ads (ampproject#7152)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lannka lannka lannka approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lannka lannka

Labels

WG: monetization

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@SteffenAnders @googlebot @jridgewell @lannka @erwinmombay