Skip to content

Add ValueCommerce support for amp-ad#6962

Merged
lannka merged 3 commits intoampproject:masterfrom
MichinaoShimizu:master
Jan 27, 2017
Merged

Add ValueCommerce support for amp-ad#6962
lannka merged 3 commits intoampproject:masterfrom
MichinaoShimizu:master

Conversation

@MichinaoShimizu
Copy link
Copy Markdown
Contributor

@MichinaoShimizu MichinaoShimizu commented Jan 10, 2017 •
edited by jridgewell
Loading

we want to add ValueCommerce into the supported ad network for amp-ad.

We have signed Google Corporate CLA.

Corporation name: ValueCommerce Co., Ltd.

Thank you very much !

Fixes #7080.

@jridgewell jridgewell requested a review from lannka January 10, 2017 19:38
@jridgewell
Copy link
Copy Markdown
Contributor

jridgewell commented Jan 10, 2017

/to @lannka

@MichinaoShimizu MichinaoShimizu mentioned this pull request Jan 18, 2017
Closed
@jridgewell
Copy link
Copy Markdown
Contributor

jridgewell commented Jan 18, 2017

Ping @lannka, @zhouyx

@jridgewell jridgewell requested a review from zhouyx January 18, 2017 18:25
@lannka lannka self-assigned this Jan 18, 2017
@lannka
Copy link
Copy Markdown
Contributor

lannka commented Jan 18, 2017

@MichinaoShimizu did you get the example working locally?

I got this error:
screenshot from 2017-01-18 12 50 47

Take a look at our guideline if you don't know how to run locally.

@iwaiwaiwa012g
Copy link
Copy Markdown
Contributor

iwaiwaiwa012g commented Jan 18, 2017

@lannka I am sorry for the inconvenience. The hostname amp.valuecommerce.com provided in the example code is under maintenance for a while, but it will be available next week. Could you wait for our update a little while? Thank you very much.

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Jan 18, 2017

no worry, take your time :-)

@iwaiwaiwa012g
Copy link
Copy Markdown
Contributor

iwaiwaiwa012g commented Jan 24, 2017 •
edited
Loading

@lannka Our server is ready now again. Could you resume the review?

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Jan 25, 2017

@iwaiwaiwa012g it works now.
However, I'm seeing a noticeable blank period between the disappear of loading indicator and display of ad. Can you confirm you code is calling the render-start API at the right time?

@MichinaoShimizu
add preconnect ValueCommerce
2ae56b0 
add preconnect ValueCommerce
@MichinaoShimizu
remove extra space
6e33e8d 
remove extra space
@iwaiwaiwa012g
Copy link
Copy Markdown
Contributor

iwaiwaiwa012g commented Jan 25, 2017

@lannka Thank you very much. It seemed that we had render-start API implemented correctly. We were missing preconnect in _config.js, though. Now it is included in the pull request as well. Could you take a look once again?

@lannka lannka merged commit c6dfcf4 into ampproject:master Jan 27, 2017
jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017
@MichinaoShimizu @jridgewell
Add ValueCommerce support for amp-ad (ampproject#6962)
f66f20b 
* 1st commit

* add preconnect ValueCommerce

add preconnect ValueCommerce

* remove extra space

remove extra space
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
@MichinaoShimizu @mrjoro
Add ValueCommerce support for amp-ad (ampproject#6962)
46be782 
* 1st commit

* add preconnect ValueCommerce

add preconnect ValueCommerce

* remove extra space

remove extra space
This was referenced Jun 14, 2017
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhouyx zhouyx Awaiting requested review from zhouyx

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.

4 participants

@MichinaoShimizu @jridgewell @lannka @iwaiwaiwa012g