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

Intent To Implement: Giraff support for amp-ads #12099

Merged
merged 3 commits into from Nov 30, 2017
Merged

Intent To Implement: Giraff support for amp-ads #12099

merged 3 commits into from Nov 30, 2017

Conversation

dvdknv
Copy link
Contributor

@dvdknv dvdknv commented Nov 17, 2017

Added support for Giraff widgets to amp-ad elements

@zhouyx zhouyx self-assigned this Nov 17, 2017
@zhouyx zhouyx added this to Ads backlog in 3P Ads & Analytics Support Nov 17, 2017
Copy link
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

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

LGTM with few comments.

ads/giraff.md Outdated

## Configuration

For more information, please see ad network documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please add link to your ad network documentation

@@ -236,6 +236,7 @@ See [amp-ad rules](https://github.com/ampproject/amphtml/blob/master/extensions/
- [Doubleclick](../../ads/google/doubleclick.md)
- [E-Planning](../../ads/eplanning.md)
- [Ezoic](../../ads/ezoic.md)
- [Giraff](../../ads/giraff.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alphabetic order.

ads/giraff.md Outdated
type="giraff"
data-block-name="novotekaru">
</amp-ad>

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove empty line

@zhouyx zhouyx moved this from Ads backlog to In Review in 3P Ads & Analytics Support Nov 18, 2017
@ampprojectbot ampprojectbot added this to the Pending Triage milestone Nov 20, 2017
* 'master' of github.com:ampproject/amphtml: (47 commits)
  Fix cloned node dupe id causing activation to fail on non images (#12241)
  pass window ref to getMode (#12239)
  Create iframe within amp-ads for inabox (#12211)
  Analytics: Timer spec provides configuration for starting and stopping (#11813)
  Validator rollup (#12242)
  amp-accordion: Add `expand`, `collapse`, `toggle` actions (#11933)
  amp-video: analytics update desc file (#12230)
  fix amp-sidebar toolbar issue with chrome when the page takes too long time to be ready (#12204)
  Web share API is out of origin trial.
  Fix heroku deployment scripts (#12236)
  Analytics enable reset visible event (#12094)
  Doubleclick render idle throttle (#12220)
  Update async image decoding attribute to updated standard. (#12145)
  dynamically insert script for unknown extensions found in live-list (#9302)
  Basic styling of bookend for desktop (#12155)
  Implement MediaPool for amp-story (#12156)
  IframeTransport + LongTask API (#11907)
  amp-layout: enable visual tests (#12158)
  Update version of eslint (#12179)
  Stop requiring source origin header for bookend (#11895)
  ...
@zhouyx
Copy link
Contributor

zhouyx commented Nov 30, 2017

Looks great! LGTM

@zhouyx zhouyx merged commit dc7b58c into ampproject:master Nov 30, 2017
@zhouyx zhouyx moved this from In Review to Done in 3P Ads & Analytics Support Nov 30, 2017
ghost pushed a commit that referenced this pull request Dec 6, 2017
* Giraff support for amp-ads

* Giraff support for amp-ads. Fixed issues
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
* Giraff support for amp-ads

* Giraff support for amp-ads. Fixed issues
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

4 participants