✨Add RnetPlus ad exchange support to amp-ad #22024

vkrapivin-sc · 2019-04-26T19:56:39Z

Add RnetPlus ad exchange support to amp-ad

This is yet another Ad Network Exchange from Rambler&Co (also known as RNet+ or RNetPlus); technically it is Russian news exchange service across Russian big sites.

googlebot · 2019-04-26T19:56:42Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

vkrapivin-sc · 2019-04-26T20:04:17Z

I signed it!

googlebot · 2019-04-26T20:04:20Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

mrjoro · 2019-05-15T21:01:41Z

Sorry for the delay in reviewing this @vkrapivin-sc! @lannka can you take a look?

/cc @ampproject/wg-ads

calebcordry

Thanks for contributing. Just a couple of nits.

calebcordry · 2019-05-15T22:26:40Z

ads/_config.js

@@ -838,6 +838,8 @@ export const adConfig = {
    renderStartImplemented: true,
  },

+  'rnetplus': {},


FYI, preload/prefetch are popular options here if you know your creative will be requesting a certain file

Thank you, but from the technical manner we have JSONP request and it cannot be precached. The images and other stuff are relied to server response and we cannot prefetch it too... Anyway I'll review code to think if we still have something to declare, thank you.

Sounds good.

calebcordry · 2019-05-15T22:26:43Z

ads/rnetplus.js

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


calebcordry · 2019-05-15T22:28:26Z

ads/rnetplus.md

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


calebcordry · 2019-05-15T22:44:36Z

examples/ads.amp.html

+  <amp-ad width="300" height="400"
+      type="rnetplus"
+      layout="responsive"
+      src="https://api.rnet.plus/Scripts/rnet_amp_embed.js?blockId=660">


It is up to you but you may want to hardcode this url, and have the block-id be a separate attribute? May be less likely for your users to break.

Actually, it is little bit corporate specific, I am sorry.

In few words, without RNetPlus team, you have no chances to place certain block to your pages with desired content; the empty blocks displays nothing. So, from the other side of this tube, somebody need to collect couple of teasers to display and assign to the particular block.

As result, RNetPlus Team is always involved into embedding the teasers and it provides the code snippets to place teasers for AMP case and for casual web cases.

It means the exact form is not important.

But if the form will be important for somebody I able to redesign this piece, thank you.

calebcordry

Thanks for contributing.

calebcordry · 2019-07-02T16:46:04Z

@vkrapivin-sc looks like you have some formatting errors. Can you run this command gulp lint --local_changes --fix and see if the output is correct and commit?

You can then run gulp lint --local_changes and make sure it passes. Let me know if you need any help. Thanks

vkrapivin-sc · 2019-07-12T11:13:38Z

Can you run this command gulp lint --local_changes --fix and see if the output is correct and commit?

Hmm, I've checked this some time; but I see no any errors / warnings for ads/ folder. Seems I need some hints.

calebcordry · 2019-07-17T16:42:17Z

@vkrapivin-sc do you have any output when you run gulp lint? Here is the output I am seeing on travis :

Looks like your editor is inserting carriage returns, and prettier wants you to remove them.

vkrapivin-sc · 2019-07-18T13:29:50Z

Looks like your editor is inserting carriage returns, and prettier wants you to remove them.

Got it, thank you. It seems dos2unix conversion helped. Sorry for this mistake.

calebcordry · 2019-07-18T16:57:50Z

No problem, thanks for contributing!

* amp-ad: adding RnetPlus ad exchange support * comments: 2019 year has been fixed * rnetplus: fixed formatting errors with CR/LF

amp-ad: adding RnetPlus ad exchange support

188c8a6

googlebot added the cla: no label Apr 26, 2019

googlebot added cla: yes and removed cla: no labels Apr 26, 2019

mrjoro requested a review from lannka May 15, 2019 21:01

calebcordry requested changes May 15, 2019

View reviewed changes

calebcordry changed the title ~~✨ Add RnetPlus ad exchange support to amp-ad~~ ✨Add RnetPlus ad exchange support to amp-ad May 15, 2019

calebcordry reviewed May 15, 2019

View reviewed changes

lannka removed their request for review May 16, 2019 15:35

lannka assigned calebcordry May 16, 2019

comments: 2019 year has been fixed

c1e8387

calebcordry approved these changes Jul 2, 2019

View reviewed changes

rnetplus: fixed formatting errors with CR/LF

b3f16e8

calebcordry merged commit 7729ae1 into ampproject:master Jul 18, 2019

rindo pushed a commit to logly/amphtml that referenced this pull request Jul 24, 2019

✨Add RnetPlus ad exchange support to amp-ad (ampproject#22024)

a54515a

* amp-ad: adding RnetPlus ad exchange support * comments: 2019 year has been fixed * rnetplus: fixed formatting errors with CR/LF

thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019

✨Add RnetPlus ad exchange support to amp-ad (ampproject#22024)

f9a953b

* amp-ad: adding RnetPlus ad exchange support * comments: 2019 year has been fixed * rnetplus: fixed formatting errors with CR/LF

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Add RnetPlus ad exchange support to amp-ad #22024

✨Add RnetPlus ad exchange support to amp-ad #22024

vkrapivin-sc commented Apr 26, 2019

googlebot commented Apr 26, 2019

vkrapivin-sc commented Apr 26, 2019

googlebot commented Apr 26, 2019

mrjoro commented May 15, 2019

calebcordry left a comment

calebcordry May 15, 2019

vkrapivin-sc Jun 7, 2019

calebcordry Jul 2, 2019

calebcordry May 15, 2019

vkrapivin-sc Jun 7, 2019

calebcordry May 15, 2019

vkrapivin-sc Jun 7, 2019

calebcordry May 15, 2019

vkrapivin-sc Jun 7, 2019

calebcordry Jul 2, 2019

calebcordry left a comment

calebcordry commented Jul 2, 2019

vkrapivin-sc commented Jul 12, 2019

calebcordry commented Jul 17, 2019

vkrapivin-sc commented Jul 18, 2019

calebcordry commented Jul 18, 2019

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

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

✨Add RnetPlus ad exchange support to amp-ad #22024

✨Add RnetPlus ad exchange support to amp-ad #22024

Conversation

vkrapivin-sc commented Apr 26, 2019

googlebot commented Apr 26, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

vkrapivin-sc commented Apr 26, 2019

googlebot commented Apr 26, 2019

mrjoro commented May 15, 2019

calebcordry 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcordry left a comment

Choose a reason for hiding this comment

calebcordry commented Jul 2, 2019

vkrapivin-sc commented Jul 12, 2019

calebcordry commented Jul 17, 2019

vkrapivin-sc commented Jul 18, 2019

calebcordry commented Jul 18, 2019