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

Engageya AMP #10237

Merged
merged 4 commits into from Aug 2, 2017
Merged

Engageya AMP #10237

merged 4 commits into from Aug 2, 2017

Conversation

reemeng
Copy link
Contributor

@reemeng reemeng commented Jul 3, 2017

Engageya AMP

Engageya AMP
@googlebot
Copy link

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.

@reemeng
Copy link
Contributor Author

reemeng commented Jul 4, 2017 via email

@techhtml
Copy link
Contributor

Check CLA

@reemeng
Copy link
Contributor Author

reemeng commented Jul 12, 2017

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@reemeng
Copy link
Contributor Author

reemeng commented Jul 12, 2017

I signed it!

@lannka lannka added this to Ads backlog in 3P Ads & Analytics Support Jul 13, 2017
@lannka lannka self-assigned this Jul 13, 2017
Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

Looks good. Some minor comments

data-widgetIds="72154"
data-websiteId="111292"
data-publisherId="155235">
</amp-embed>
Copy link
Contributor

Choose a reason for hiding this comment

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

could you follow the indentation in this file?

@@ -104,6 +104,7 @@
<option>distroscale</option>
<option>dotandads</option>
<option>doubleclick</option>
<option>Engageya</option>
Copy link
Contributor

Choose a reason for hiding this comment

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

pls fix the indentation

limitations under the License.
-->

# Engageya
Copy link
Contributor

Choose a reason for hiding this comment

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

would you provide a reference link somewhere to your site / specs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need for now

@lannka lannka moved this from Ads backlog to In Review in 3P Ads & Analytics Support Jul 13, 2017
@lannka
Copy link
Contributor

lannka commented Jul 20, 2017

@reemeng a kind ping. would you address the comments so we can get it merged :-)

@lannka lannka merged commit c4273ca into ampproject:master Aug 2, 2017
@lannka lannka moved this from In Review to Done in 3P Ads & Analytics Support Aug 7, 2017
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.

None yet

6 participants