Add a8ad by szk-fancs · Pull Request #8036 · ampproject/amphtml

szk-fancs · 2017-03-09T11:15:09Z

Add A8 ad network for amp-ad.
Corporation name: F@NCommunications,Inc

googlebot · 2017-03-09T11:15:12Z

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.

szk-fancs · 2017-03-09T11:18:32Z

Add A8 ad network for amp-ad.
Corporation name: F@NCommunications,Inc

googlebot · 2017-03-09T11:18:34Z

CLAs look good, thanks!

szk-fancs · 2017-03-14T02:46:08Z

Ping @lannka, @zhouyx
Sorry for ping you.
Please review PR.

zhouyx · 2017-03-14T17:47:49Z

3p/integration.js

  }
  for (let i = 0; i < allowedHostnames.length; i++) {
-    // Either the hostname is exactly as whitelisted…
+    // Either the hostname is exactly as whitelisted...


revert change here.

zhouyx · 2017-03-14T17:48:05Z

ads/a8.js

@@ -0,0 +1,27 @@
+/**
+ * Copyright 2016 The AMP HTML Authors. All Rights Reserved.


zhouyx · 2017-03-14T17:48:16Z

ads/a8.md

@@ -0,0 +1,36 @@
+<!---
+Copyright 2016 The AMP HTML Authors. All Rights Reserved.


zhouyx · 2017-03-14T17:49:42Z

ads/a8.md

+	data-mat="2TCGYR-17GNXU-3L92-64Z8X"
+	data-type="static"
+>
+</amp-ad>```


git won't recognize ``` in same line. new line instead

zhouyx · 2017-03-14T17:50:46Z

ads/a8.md

+>
+</amp-ad>```
+
+## Configuration


So data-aid is required, while other 5 params are optional. Let's document this here.

zhouyx · 2017-03-14T17:51:07Z

examples/ads.amp.html

  </div>

+  <h2 id="a8">A8</h2>
+<amp-ad width="300" height="250"


indentation please

zhouyx · 2017-03-14T17:51:42Z

examples/ads.amp.html

+	data-mid="s00000016751001031000"
+	data-mat="2TCGYR-17GNXU-3L92-64Z8X"
+	data-type="static"
+>


same line please

zhouyx · 2017-03-14T17:52:53Z

ads/a8.md

@@ -0,0 +1,36 @@
+<!---
+Copyright 2016 The AMP HTML Authors. All Rights Reserved.


zhouyx · 2017-03-14T17:53:01Z

ads/a8.js

@@ -0,0 +1,27 @@
+/**
+ * Copyright 2016 The AMP HTML Authors. All Rights Reserved.


ampprojectbot · 2017-03-14T17:53:22Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko

3p/integration.js
extensions/amp-ad/amp-ad.md

/to lannka zhouyx

ads/_config.js
ads/a8.js
ads/a8.md

/to camelburrito lannka

examples/ads.amp.html

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

szk-fancs · 2017-03-15T02:32:40Z

@zhouyx Thank you for review! Oh...I'm so sorry. My code was not clean...
And I misunderstood it as a copyright notation of the project.
Because "year" is not usually updated.
I will fix it.

Cleaned indent. Add params description.

szk-fancs · 2017-03-15T09:28:41Z

@zhouyx
Fixed it.
Please check it, whenever convenient for you!
Thank you.

zhouyx

LGTM with two more minor requests

zhouyx · 2017-03-16T23:20:55Z

ads/a8.js

+ * @param {!Object} data
+ */
+export function a8(global, data) {
+  validateData(data, ['aid'], ['wid','eno','mid','mat','type']);


nit: space after ,

zhouyx · 2017-03-16T23:23:11Z

ads/a8.md

+## Configuration
+
+Required:data-aid
+Optional:data-wid,data-eno,data-mid,data-mat,data-type


nit: Please view the doc on git. You will notice they are in the same line. Please fix

How about writing like this.

Required parameter:

data-aid

Optional parameters:

data-wid

data-eno

data-mid

data-mat

data-type

looks great

ampprojectbot · 2017-03-16T23:23:32Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko jridgewell

3p/integration.js

/to lannka zhouyx

ads/_config.js
ads/a8.js
ads/a8.md

/to camelburrito lannka

examples/ads.amp.html

/to lannka zhouyx

extensions/amp-ad/amp-ad.md

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

szk-fancs · 2017-03-21T02:20:57Z

@zhouyx

Thank you for your review!
I fixed it.

It's my first PR to open source project.
But I realized that my consideration was not enough at all.

You are really good at me.
Thank you my first reviewer!

zhouyx · 2017-03-24T17:10:06Z

@szk-fancs Thanks for the Pull Request! You are very welcome! This PR looks great, Merged.

* master: (34 commits) Prevent amp-carousel next/previous icons fade away on desktop (ampproject#8428) Turn on flag slidescroll-disable-css-snap” (ampproject#8436) Revert "temporarily turn off yarn (ampproject#8356)" (ampproject#8384) initial commit (ampproject#8404) Upgrades for Index Exchange amp-ad tags to report load statistics (ampproject#8054) amp-bind validation tweak (ampproject#8414) Fix an amp-instagram race condition (ampproject#8192) Use whitelist to restrict urlReplacement for scoped analytics element (ampproject#8360) Report active experiments in error logs (ampproject#8108) amp-bind: Catch exceptions in mutatedAttributesCallback (ampproject#8383) Fixing custom scroll-snap on IOS (ampproject#8391) Add experiment for using AmpContext class in integration.js (ampproject#8348) add (ampproject#8349) swipe api (ampproject#8357) skip 3 flaky tests (ampproject#8388) amp-bind: Expression complexity limit (ampproject#8321) add margin-bottom (ampproject#8350) Flying carpet: make container full viewport and center content (ampproject#8292) Service Registration: Document Click (ampproject#7882) Add a8ad (ampproject#8036) ...

* Support ads “A8” . * Comment change. * Add “renderStartImplemented=true”. * Changed copyrights “2017”. Cleaned indent. Add params description. * Changed writing format.

szk-fancs added 2 commits March 9, 2017 17:47

Support ads “A8” .

94e0302

Comment change.

e491dec

szk-fancs closed this Mar 9, 2017

szk-fancs reopened this Mar 9, 2017

Add “renderStartImplemented=true”.

c7a6b2e

zhouyx suggested changes Mar 14, 2017

View reviewed changes

Changed copyrights “2017”.

73b96e3

Cleaned indent. Add params description.

jridgewell added the WG: monetization label Mar 15, 2017

zhouyx reviewed Mar 16, 2017

View reviewed changes

Changed writing format.

7da2848

zhouyx approved these changes Mar 24, 2017

View reviewed changes

zhouyx merged commit d9ff92b into ampproject:master Mar 24, 2017

ampprojectbot added the PR use: In Beta / Experimental label Apr 4, 2017

szk-fancs deleted the addA8ad branch April 5, 2017 07:44

		@@ -0,0 +1,27 @@
		/**
		* Copyright 2016 The AMP HTML Authors. All Rights Reserved.

		@@ -0,0 +1,36 @@
		<!---
		Copyright 2016 The AMP HTML Authors. All Rights Reserved.

Conversation

szk-fancs commented Mar 9, 2017

Uh oh!

googlebot commented Mar 9, 2017

Uh oh!

szk-fancs commented Mar 9, 2017

Uh oh!

googlebot commented Mar 9, 2017

Uh oh!

szk-fancs commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ampprojectbot commented Mar 14, 2017

Uh oh!

szk-fancs commented Mar 15, 2017

Uh oh!

szk-fancs commented Mar 15, 2017

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ampprojectbot commented Mar 16, 2017

Uh oh!

szk-fancs commented Mar 21, 2017

Uh oh!

zhouyx commented Mar 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants