Add Unruly ad network #14828

davidalk · 2018-04-24T15:16:01Z

Executed all the test as per instructions page and performed the linting and type checks. We have also implemented renderStart as per the advice in the pull request guide.

This is a re-raised PR so the following can be closed:

#14724

This PR has the correct verified author and squashed commits.

davidalk · 2018-04-26T16:19:03Z

Hi, is there anything else required from our side for approval?

Thanks.

jpettitt · 2018-05-08T14:29:33Z

I've added a couple of reviewers and restarted your CI checks.

cc @calebcordry @lannka

davidalk · 2018-05-14T16:57:57Z

Hi, any update on this PR?

Thanks.

calebcordry

Thanks for contributing! A few small things.

calebcordry · 2018-05-08T14:40:07Z

ads/unruly.js

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


calebcordry · 2018-05-08T14:40:28Z

ads/unruly.md

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


calebcordry · 2018-05-15T16:27:26Z

ads/unruly.md

+```html
+<amp-ad width="620" height="349"
+    type="unruly"
+    date-siteId="xxx">


Is this a mandatory attribute? If so could you document it as so?

Typo? date-siteId or data-siteId

nit: prefer data-site-id for consistency.

calebcordry · 2018-05-15T16:27:56Z

examples/ads.amp.html

@@ -1140,7 +1141,7 @@ <h2>Monetizer101</h2>
      type="monetizer101"
      data-widget="price-comparison"
      data-config='{"shopId": 1, "priceMin": 500, "nameKeywords": "iphone"}'>
-  </amp-ad>  


calebcordry · 2018-05-15T16:32:55Z

ads/unruly.js

+
+export function unruly(global, data, scriptLoader = loadScript) {
+  global.unruly = global.unruly || {};
+  global.unruly.native = {


You may want to validate the input here. We have this function to help:

amphtml/3p/3p.js

Lines 213 to 227 in d86f11f

/**

* Validates given data. Throws an exception if the data does not

* contains a mandatory field. If called with the optional param

* opt_optionalFields, it also validates that the data contains no fields other

* than mandatory and optional fields.

*

* Mandatory fields also accept a string Array as an item. All items in that

* array are considered as alternatives to each other. So the validation checks

* that the data contains exactly one of those alternatives.

*

* @param {!Object} data

* @param {!Array<string|!Array<string>>} mandatoryFields

* @param {Array<string>=} opt_optionalFields

*/

export function validateData(data, mandatoryFields, opt_optionalFields) {

davidalk · 2018-05-16T10:41:27Z

Hi @calebcordry, thanks for your review, we've made the suggested changes and pushed.

calebcordry · 2018-05-17T03:12:10Z

test/functional/ads/test-unruly.js

+    const mockGlobal = {};
+    let expectedGlobal;
+    let expectedUrl;
+    const scriptLoader = (...args) => {


this spread may fail lint (we sometimes have frustrating lint rules) Travis is very backed up today due to some new changes, so let's let it finish and see what happens.

edit: rest, not spread, but same idea

calebcordry · 2018-05-17T03:14:05Z

Looking good, you will also need to rebase due to merge conflicts. Let me know if you need help with the process.

calebcordry · 2018-05-17T15:51:15Z

Travis appears to have borked here. When you rebase it should restart it anyway.

davidalk · 2018-05-21T11:06:55Z

Have fixed the conflicts and pushed.

davidalk · 2018-05-21T11:55:30Z

@calebcordry, its failing on the Percy tests, do you know how I can check these? I don't have permission when I select the details link.

davidalk · 2018-05-22T23:26:59Z

Have got access to Percy and can see the diff was on the amp-video.amp.html page which isn't affected by this PR. Please can you go ahead with the merge? Thanks.

calebcordry · 2018-05-22T23:31:40Z

Looks good. Thanks!

googlebot added the cla: yes label Apr 24, 2018

jpettitt requested a review from lannka May 8, 2018 14:27

jpettitt added the WG: monetization label May 8, 2018

jpettitt requested a review from calebcordry May 8, 2018 14:28

calebcordry self-assigned this May 15, 2018

calebcordry requested changes May 15, 2018

View reviewed changes

calebcordry removed the request for review from lannka May 15, 2018 16:31

calebcordry reviewed May 15, 2018

View reviewed changes

calebcordry reviewed May 17, 2018

View reviewed changes

ghost force-pushed the unruly-amp-pr branch from 33dc3af to c75f2ca Compare May 18, 2018 09:49

calebcordry added this to TODO in 3P Ads & Analytics Support via automation May 18, 2018

calebcordry moved this from TODO to In Review in 3P Ads & Analytics Support May 18, 2018

feat(amp-ads): Add Unruly Ad Network

52df407

ghost force-pushed the unruly-amp-pr branch from 133ad07 to 52df407 Compare May 21, 2018 11:04

calebcordry approved these changes May 22, 2018

View reviewed changes

calebcordry merged commit 4b361c3 into ampproject:master May 22, 2018

3P Ads & Analytics Support automation moved this from In Review to Done May 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Unruly ad network #14828

Add Unruly ad network #14828

davidalk commented Apr 24, 2018

davidalk commented Apr 26, 2018

jpettitt commented May 8, 2018

davidalk commented May 14, 2018

calebcordry left a comment

calebcordry May 8, 2018

calebcordry May 8, 2018

calebcordry May 15, 2018

jpettitt May 15, 2018

calebcordry May 15, 2018

calebcordry May 15, 2018

davidalk commented May 16, 2018

calebcordry May 17, 2018 •

edited

calebcordry commented May 17, 2018

calebcordry commented May 17, 2018

davidalk commented May 21, 2018

davidalk commented May 21, 2018

davidalk commented May 22, 2018

calebcordry commented May 22, 2018

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

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

	/**
	* Validates given data. Throws an exception if the data does not
	* contains a mandatory field. If called with the optional param
	* opt_optionalFields, it also validates that the data contains no fields other
	* than mandatory and optional fields.
	*
	* Mandatory fields also accept a string Array as an item. All items in that
	* array are considered as alternatives to each other. So the validation checks
	* that the data contains exactly one of those alternatives.
	*
	* @param {!Object} data
	* @param {!Array<string\|!Array<string>>} mandatoryFields
	* @param {Array<string>=} opt_optionalFields
	*/
	export function validateData(data, mandatoryFields, opt_optionalFields) {

Add Unruly ad network #14828

Add Unruly ad network #14828

Conversation

davidalk commented Apr 24, 2018

davidalk commented Apr 26, 2018

jpettitt commented May 8, 2018

davidalk commented May 14, 2018

calebcordry left a comment

Choose a reason for hiding this comment

calebcordry May 8, 2018

Choose a reason for hiding this comment

calebcordry May 8, 2018

Choose a reason for hiding this comment

calebcordry May 15, 2018

Choose a reason for hiding this comment

jpettitt May 15, 2018

Choose a reason for hiding this comment

calebcordry May 15, 2018

Choose a reason for hiding this comment

calebcordry May 15, 2018

Choose a reason for hiding this comment

davidalk commented May 16, 2018

calebcordry May 17, 2018 • edited

Choose a reason for hiding this comment

calebcordry commented May 17, 2018

calebcordry commented May 17, 2018

davidalk commented May 21, 2018

davidalk commented May 21, 2018

davidalk commented May 22, 2018

calebcordry commented May 22, 2018

calebcordry May 17, 2018 •

edited