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

amp-ad type=dotandads implementation #1705

Merged
merged 1 commit into from Feb 3, 2016
Merged

amp-ad type=dotandads implementation #1705

merged 1 commit into from Feb 3, 2016

Conversation

dotandads
Copy link
Contributor

Provides support for amp-ad type=dotandads

@dotandads
Copy link
Contributor Author

I think there are some problems with lint. It's returning "TypeError: Cannot read property 'visitClass' of undefined" in Travis Build

@cramforce
Copy link
Member

Do you see the same error when running gulp lint locally?

<style>body {opacity: 0}</style><noscript><style>body {opacity: 1}</style></noscript>
</head>
<body>
<amp-ad width=980 height=250
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I missed this before: Please merge this into ads.amp.html. We're trying to keep the count of examples somewhat limited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll merge them

@cramforce
Copy link
Member

Running lint on a clean branch to check whats up https://travis-ci.org/ampproject/amphtml/builds/106271563

@dotandads
Copy link
Contributor Author

Yes, I have the same error locally

@cramforce
Copy link
Member

Confirmed it is not your issue https://travis-ci.org/ampproject/amphtml/builds/106271563

Please address the example-comment above and we can merge this. Thanks!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot
Copy link

CLAs look good, thanks!

@dotandads
Copy link
Contributor Author

Ok, I've fixed the example and squashed the commits ^^

Thank you

@dotandads
Copy link
Contributor Author

Do we have to make some fixes for the conflicts that the check is showing?

@cramforce
Copy link
Member

@dotandads You do need to rebase from master, so we can merge your PR. After that the build should also be green.

added implementation for dotandads type for amp-ad

fix merge

merge fix

fix merge

fix merge
@dotandads
Copy link
Contributor Author

@cramforce ok, thanks. Everything looks green now


## Configuration

For semantics of configuration, please see ad network documentation.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe in a follow up send a PR with a link, so people know where to look.

@cramforce
Copy link
Member

LGTM. See comment above for a follow up. Merging.

@cramforce cramforce added the LGTM label Feb 3, 2016
cramforce added a commit that referenced this pull request Feb 3, 2016
amp-ad type=dotandads implementation
@cramforce cramforce merged commit dc14548 into ampproject:master Feb 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants