adding sekindo ads #11935

nissSK · 2017-11-05T14:26:08Z

No description provided.

googlebot · 2017-11-05T14:26:11Z

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 your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-11-06T12:53:49Z

CLAs look good, thanks!

nissSK · 2017-11-20T08:49:55Z

Hello Amphtml team,
Should we do any additional action in order for our pull request to be reviewed?

Thanks,

nissSK · 2018-01-23T14:09:28Z

I signed it!

zhouyx

@nissSK Sorry for the delay here!
The Pull Requests looks good. I left few comments.

zhouyx · 2018-03-02T18:39:26Z

ads/_config.js

@@ -623,6 +623,9 @@ export const adConfig = {

  rubicon: {},

+  sekindo: {
+    renderStartImplemented: true,
+  },


nit: new line

zhouyx · 2018-03-02T18:39:56Z

ads/sekindo.js

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


zhouyx · 2018-03-02T18:41:37Z

ads/sekindo.js

+export function sekindo(global, data) {
+  validateData(data, ['spaceid']);
+  const pubUrl = encodeURIComponent(global.context.sourceUrl);
+  const excluedsSet = {ampSlotIndex: 1, type: 1};


zhouyx · 2018-03-02T18:42:10Z

ads/sekindo.js

+      }
+    }
+  }
+  loadScript(global, 'https://live.sekindo.com/live/liveView.php?' + query, () => {


nit: 80 chars limit

zhouyx · 2018-03-02T18:42:19Z

ads/sekindo.md

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


zhouyx · 2018-03-02T18:45:08Z

examples/ads.amp.html

@@ -1194,6 +1194,12 @@ <h2>Rubicon Project Smart Tag</h2>
      json='{"visitor":{"age":"18-24","gender":"male"},"inventory":{"section":"amp"}}'>
  </amp-ad>

+  <h2>Sekindo</h2>


Please add your example to this form as well.
Also please sort them in alphabetic order. This goes after rubicon

zhouyx · 2018-03-02T18:47:06Z

ads/sekindo.md

+
+__Required:__
+
+- `data-spaceId` - Adunit unique id


Just FYI:
data-spaceId will be converted to data['spaceid']
data-space-id will be converted to data['spaceId']

sekindo · 2018-03-15T08:33:55Z

@zhouyx Happy to see us in the queue :)
I committed the fixes according to your CR comments,
Thanks,

zhouyx

@sekindo Looks good to me.
To fix the percy test please rebase to master.
Travis fail because of lint check. Please fix indentation and sort function names in alphabetical order. You can use gulp lint to check, and gulp lint --fix for a quick auto fix.

zhouyx · 2018-03-17T01:28:58Z

ads/sekindo.js

+    }
+  }
+  loadScript(global, 
+  'https://live.sekindo.com/live/liveView.php?' + query, () => {


please fix indentation to pass the lint check.

zhouyx · 2018-03-17T01:29:23Z

ads/sekindo.js

+ * limitations under the License.
+ */
+
+import {validateData, loadScript} from '../3p/3p';


please sort in alphabetical order to fix lint check

…ekindo_1

sekindo · 2018-03-18T12:10:12Z

@zhouyx , We committed the needed changes, I see that we are still failing on travis But I cant see how it's related to our changes.

zhouyx · 2018-03-19T22:40:53Z

@sekindo Seems like a flaky test. All tests passed! Thanks for the Pull Request. Merged!

nissSK added 3 commits November 5, 2017 16:11

adding sekindo ads

63755b4

Merge branch 'master' of https://github.com/ampproject/amphtml

2ca8775

new line

26a1084

googlebot added the cla: no label Nov 5, 2017

remove extra space

eb02707

googlebot added cla: yes and removed cla: no labels Nov 6, 2017

ampprojectbot added this to the Pending Triage milestone Nov 6, 2017

removing newline

14d35c4

cathyxz added this to TODO in 3P Ads & Analytics Support via automation Feb 23, 2018

cathyxz requested review from zhouyx and lannka February 23, 2018 19:14

zhouyx suggested changes Mar 2, 2018

View reviewed changes

zhouyx self-assigned this Mar 2, 2018

zhouyx moved this from TODO to In Review in 3P Ads & Analytics Support Mar 2, 2018

lannka removed their request for review March 3, 2018 23:22

fixing code review comments

ce4f324

zhouyx approved these changes Mar 17, 2018

View reviewed changes

nissSK added 3 commits March 18, 2018 10:46

coding style

bb9f3e0

Merge branch 'master' of https://github.com/ampproject/amphtml into s…

df6bca3

…ekindo_1

coding style

6290e03

zhouyx merged commit ce5c99d into ampproject:master Mar 19, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding sekindo ads #11935

adding sekindo ads #11935

nissSK commented Nov 5, 2017

googlebot commented Nov 5, 2017

googlebot commented Nov 6, 2017

nissSK commented Nov 20, 2017

nissSK commented Jan 23, 2018

zhouyx left a comment

zhouyx Mar 2, 2018

zhouyx Mar 2, 2018

zhouyx Mar 2, 2018

zhouyx Mar 2, 2018

zhouyx Mar 2, 2018

zhouyx Mar 2, 2018

zhouyx Mar 2, 2018

sekindo commented Mar 15, 2018

zhouyx left a comment

zhouyx Mar 17, 2018

zhouyx Mar 17, 2018

sekindo commented Mar 18, 2018

zhouyx commented Mar 19, 2018

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

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

adding sekindo ads #11935

adding sekindo ads #11935

Conversation

nissSK commented Nov 5, 2017

googlebot commented Nov 5, 2017

googlebot commented Nov 6, 2017

nissSK commented Nov 20, 2017

nissSK commented Jan 23, 2018

zhouyx left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sekindo commented Mar 15, 2018

zhouyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sekindo commented Mar 18, 2018

zhouyx commented Mar 19, 2018