Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

myWidget Ad extension #7471

Merged
merged 4 commits into from Feb 16, 2017
Merged

myWidget Ad extension #7471

merged 4 commits into from Feb 16, 2017

Conversation

3lvcz
Copy link
Contributor

@3lvcz 3lvcz commented Feb 10, 2017

Pull-request for "myWidget" ad.

Closes #6965

(broken closed one #7252)

@jridgewell
Copy link
Contributor

/to @lannka, @zhouyx

@lannka lannka removed their request for review February 11, 2017 05:52
@lannka
Copy link
Contributor

lannka commented Feb 11, 2017

@3lvcz pls fix travis

@3lvcz
Copy link
Contributor Author

3lvcz commented Feb 13, 2017

I fixed previous build error (missed property annotation), but it fails on tests now. It's our code is broken?

@jridgewell
Copy link
Contributor

Restarted the test build. Please check back in a bit.

ads/mywidget.js Outdated
@@ -0,0 +1,78 @@
/**
* Copyright 2015 The AMP HTML Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 2017

ads/mywidget.md Outdated
@@ -0,0 +1,40 @@
<!---
Copyright 2015 The AMP HTML Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 2017

ads/mywidget.js Outdated
@@ -0,0 +1,78 @@
/**
* Copyright 2015 The AMP HTML Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 2017

ads/mywidget.md Outdated
@@ -0,0 +1,40 @@
<!---
Copyright 2015 The AMP HTML Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

ads/mywidget.md Outdated

Supported parameters:

- `data-cid`
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that data-cid is a required parameter, I would suggest indicate that it is required here.

@zhouyx
Copy link
Contributor

zhouyx commented Feb 15, 2017

Few comments. LGTM in general

@3lvcz
Copy link
Contributor Author

3lvcz commented Feb 16, 2017

Fixed year in license headers and described cid as required attribute in mywidget.md

@zhouyx zhouyx merged commit e606836 into ampproject:master Feb 16, 2017
@zhouyx
Copy link
Contributor

zhouyx commented Feb 16, 2017

@3lvcz Thanks for the Pull Request. Merged.

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* myWidget Ad extension

* myWidget Ad extension - fix travis

* myWidget Ad extension - fix travis

* myWidget Ad extension - review fixes
@priver priver deleted the ads-mywidget branch September 12, 2017 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Intent to implement: mywidget for amp-ad
4 participants