admanmedia ad network type added #8052

luciancrasovan · 2017-03-10T11:09:53Z

AdMan Media ad network type implementation

This implements basic amp-ad for admanmedia with a single data parameter
Unique size ads: 300 X 250
No layout configuration
No viewability control

googlebot · 2017-03-10T11:09:55Z

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.

luciancrasovan · 2017-03-10T11:24:01Z

I signed it!

googlebot · 2017-03-10T11:24:03Z

CLAs look good, thanks!

luciancrasovan · 2017-03-10T11:57:14Z

It seems CI build failed but I cannot see a relation with my changes. Please confirm and let me know which would be the solution. Thanks in advance!

luciancrasovan · 2017-03-13T14:14:17Z

This is for the AdMan Media company http://admanmedia.com/
It urges us to see it available in the Repository in order to be able to test it with real clients so a fast acceptance of this pull request will be highly appreciated!

lcrasovan · 2017-03-17T10:52:54Z

Hello,

Is any news about this pull request?

Thanks in advance

jasti · 2017-03-27T13:12:48Z

@lannka @zhouyx would you please review this PR? Thanks.

zhouyx · 2017-05-04T22:56:11Z

ads/_config.js

@@ -75,6 +75,8 @@ export const adConfig = {

  adman: {},

+  admanmedia: {},


We have a render-start API that helps to provide better user experience. Please consider implementing it. Please let me know if there's any questions. doc

zhouyx · 2017-05-04T22:56:19Z

ads/admanmedia.js

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


zhouyx · 2017-05-04T22:57:00Z

ads/admanmedia.js

+ * @param {!Object} data
+ */
+export function admanmedia(global, data) {
+  validateData(data, ['id'], []);


third param is optional. No need to specify.

zhouyx · 2017-05-04T22:57:24Z

ads/admanmedia.md

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


zhouyx · 2017-05-04T22:58:43Z

ads/admanmedia.js

+export function admanmedia(global, data) {
+  validateData(data, ['id'], []);
+
+  const script = global.document.createElement('script');


Please refer to writeScript which is sync or loadScript which is async.

zhouyx · 2017-05-04T22:59:43Z

examples/ads.amp.html

@@ -247,6 +248,12 @@ <h2 id="adman">Adman</h2>
      data-host="talos.adman.gr">
  </amp-ad>

+  <h2 id="admanmedia">AdmanMedia</h2>
+  <amp-ad width="300" height="250"
+          type="admanmedia"


please fix indentation.

I'm not able to see a rendered example. Could you please check that the example ad is rendering?

lcrasovan · 2017-05-18T14:11:21Z

Hello @zhouyx , we have just uploaded our changes. Could you please review and let us know if everything is ok?

zhouyx · 2017-05-18T21:28:46Z

ads/admanmedia.md

+<amp-ad width="300" height="250"
+    type="admanmedia"
+    data-id="8e916419">
+  </amp-ad>


nit: indentation here.

zhouyx · 2017-05-18T21:29:38Z

ads/admanmedia.js

+export function admanmedia(global, data) {
+  validateData(data, ['id']);
+
+  loadScript(global, `https://mona.admanmedia.com/go?id=${data.id}`, () => {


please use encodeURIComponent(data.id)

zhouyx · 2017-05-18T21:31:58Z

examples/ads.amp.html

@@ -265,6 +266,12 @@
      data-host="talos.adman.gr">
  </amp-ad>

+  <h2>AdmanMedia</h2>
+  <amp-ad width="300" height="250"


I get error when testing locally. Could you please take a look?

When testing locally, as the mona.admanmedia.com does not set specially CORS headers for localhost, I am launching Chrome like this: ./chrome --disable-web-security --user-data-dir
Prior to launching it to production we will setup CORS headers on our server and this error will not be launched anymore

Cool I am able to see it now. After this PR has been merged it usually takes two weeks for it to hit prod. It's usually towards the end of a week, but not always on an exact data. Is there a concern why you can't setup the header a bit earlier?

Thanks for you valuable remarks that helped us improve the code quality. We have started the internal process for CORS header setting and hopefully in a few days we will be all set.

zhouyx · 2017-05-23T22:17:33Z

ads/admanmedia.js

+
+  const encodedId = encodeURIComponent(data.id);
+  loadScript(global, `https://mona.admanmedia.com/go?id=${encodedId}`, () => {
+    const pattern = `script[src$="id=${data.id}"]`;


encodedId here as well?

zhouyx · 2017-05-23T22:21:08Z

Look good in general. Few minor requests.

zhouyx · 2017-05-26T17:03:13Z

LGTM. Thanks for the Pull Request. Merged.

admanmedia ad network type added

bda8207

Merge remote-tracking branch 'upstream/master'

3835b41

zhouyx added this to Backlog (Should be Closed) in 3P Ads & Analytics Support May 1, 2017

zhouyx self-assigned this May 4, 2017

zhouyx suggested changes May 4, 2017

View reviewed changes

zhouyx moved this from Backlog to In Review in 3P Ads & Analytics Support May 4, 2017

Lucian Crasovan added 2 commits May 5, 2017 11:47

Merge remote-tracking branch 'upstream/master'

7373aa9

Render start implemented and demo playing ad introduced

8cafbc7

googlebot added the cla: yes label May 17, 2017

Lucian Crasovan added 3 commits May 17, 2017 15:35

Merge remote-tracking branch 'upstream/master'

7ab3882

Solve ESLint 80 chars line limit

5752551

Merge remote-tracking branch 'upstream/master'

077d34a

zhouyx suggested changes May 18, 2017

View reviewed changes

Lucian Crasovan added 2 commits May 19, 2017 13:32

Merge remote-tracking branch 'upstream/master'

1bb840e

Fix indentation and encodeURI id

8c6d37a

jridgewell added the WG: monetization label May 19, 2017

zhouyx suggested changes May 23, 2017

View reviewed changes

Lucian Crasovan added 2 commits May 24, 2017 01:26

Merge remote-tracking branch 'upstream/master'

00b9075

add encodedId in search pattern

0ba275f

zhouyx approved these changes May 26, 2017

View reviewed changes

zhouyx merged commit 82bdc49 into ampproject:master May 26, 2017

zhouyx moved this from In Review to Done in 3P Ads & Analytics Support May 26, 2017

ampprojectbot added the PR use: In Beta / Experimental label Jun 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

admanmedia ad network type added #8052

admanmedia ad network type added #8052

luciancrasovan commented Mar 10, 2017

googlebot commented Mar 10, 2017

luciancrasovan commented Mar 10, 2017

googlebot commented Mar 10, 2017

luciancrasovan commented Mar 10, 2017

luciancrasovan commented Mar 13, 2017

lcrasovan commented Mar 17, 2017

jasti commented Mar 27, 2017

zhouyx May 4, 2017

zhouyx May 4, 2017

zhouyx May 4, 2017

zhouyx May 4, 2017

zhouyx May 4, 2017

zhouyx May 4, 2017

zhouyx May 4, 2017

lcrasovan commented May 18, 2017

zhouyx May 18, 2017

zhouyx May 18, 2017

zhouyx May 18, 2017

lcrasovan May 19, 2017

zhouyx May 23, 2017 •

edited

lcrasovan May 24, 2017

zhouyx May 23, 2017

zhouyx commented May 23, 2017

zhouyx commented May 26, 2017

		@@ -75,6 +75,8 @@ export const adConfig = {

		adman: {},

		admanmedia: {},

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

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

admanmedia ad network type added #8052

admanmedia ad network type added #8052

Conversation

luciancrasovan commented Mar 10, 2017

googlebot commented Mar 10, 2017

luciancrasovan commented Mar 10, 2017

googlebot commented Mar 10, 2017

luciancrasovan commented Mar 10, 2017

luciancrasovan commented Mar 13, 2017

lcrasovan commented Mar 17, 2017

jasti commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcrasovan commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx May 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx commented May 23, 2017

zhouyx commented May 26, 2017

zhouyx May 23, 2017 •

edited