Skip to content

Initial revision of Cxense Display amp-ad integration#7333

Merged
jridgewell merged 4 commits intoampproject:masterfrom
samuel-palmer-cxense:cxense-display-integration
Feb 13, 2017
Merged

Initial revision of Cxense Display amp-ad integration#7333
jridgewell merged 4 commits intoampproject:masterfrom
samuel-palmer-cxense:cxense-display-integration

Conversation

@samuel-palmer-cxense
Copy link
Copy Markdown
Contributor

This is an integration for the Cxense Display (formerly Emediate Ad) ad server.

Details

This is a minimal implementation that uses writeScript() to fetch the "real" integration script, for more flexibility:

https://amp.emediate.eu/amp.v0.js

The reason the name "eas" is used for type, etc, is because that abbreviation is used in all of our APIs, etc, due to historical reasons.

@googlebot
Copy link
Copy Markdown

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.

@samuel-palmer-cxense
Copy link
Copy Markdown
Contributor Author

samuel-palmer-cxense commented Feb 3, 2017 via email

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@jridgewell
Copy link
Copy Markdown
Contributor

/to @lannka

@lannka lannka requested review from zhouyx and removed request for lannka February 6, 2017 22:55
zhouyx
zhouyx previously requested changes Feb 7, 2017
import {dotandads} from '../ads/dotandads';
import {doubleclick} from '../ads/google/doubleclick';
import {eplanning} from '../ads/eplanning';
import {eas} from '../ads/eas';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alphabetical order please

register('dotandads', dotandads);
register('doubleclick', doubleclick);
register('eplanning', eplanning);
register('eas', eas);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@@ -0,0 +1,592 @@
/**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this file.

# Cxense Display ad server (formerly Emediate Ad)

## Supported parameters in the amp-ad tag

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please correct the format here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, changed title to just "CxenseDisplay" instead, I hope that was what you where referring to (otherwise let me know what's wrong)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't mean the title.
I understand that you want to put a table of supported parameters here, but if you take a look of the file you will find that the format is messed up.

The easiest way is to get rid of the table. One example for you reference.
https://github.com/ampproject/amphtml/blob/master/ads/google/adsense.md

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, finally got the point.. table is removed

- [Dot and Media](../../ads/dotandads.md)
- [Doubleclick](../../ads/google/doubleclick.md)
- [E-Planning](../../ads/eplanning.md)
- [CxenseDisplay](../../ads/eas.md)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alphabetical order

ads/eas.md Outdated

## Configuration

For `data-eas-domain=`<YourAdServerDomain>`, use your ad-server domain (e. g. eas3.emediate.se); If you're using a custom domain-name (like eas.somesite.com) you should NOT use that one unless you already have an SSL-certificate installed on our ad servers.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: mismatch of `

@samuel-palmer-cxense
Copy link
Copy Markdown
Contributor Author

Sorry for that mistakes, now it should hopefully look better!

* @param {!Object} data
*/
export function eas(global, data) {
global.easAmpParams = data;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since data-eas-domain is required, I would suggest adding validateData(data, ['eas-domain']) here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented using 'easDomain' because of the name-conversion

ads/eas.md Outdated
data-eas-cu="12345"
data-eas-EASTsomename="somevalue"
data-eas-kw1="somekeyword"
>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move to the previous line

@samuel-palmer-cxense
Copy link
Copy Markdown
Contributor Author

Ok, here's next attempt, validateData() is used and the documentation should have the right format.

@zhouyx
Copy link
Copy Markdown
Contributor

zhouyx commented Feb 13, 2017

LGTM ping @lannka

@jridgewell
Copy link
Copy Markdown
Contributor

Merging since both of you LGTM'd.

torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Feb 14, 2017
* Initial revision of Cxense Display amp-ad integration

* Fixes based upon review-feedback

* Fix of another alphabetic order mishap

* Use validateData() for required ad-parameter + documentation-fixes
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* Initial revision of Cxense Display amp-ad integration

* Fixes based upon review-feedback

* Fix of another alphabetic order mishap

* Use validateData() for required ad-parameter + documentation-fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants