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-ima-video: Markdown README for project #9336

Merged
7 commits merged into from May 29, 2017
Merged

amp-ima-video: Markdown README for project #9336

7 commits merged into from May 29, 2017

Conversation

shawnbuso
Copy link
Contributor

Adds md file for amp-ima-video project.

Fixed IMA SDK link.

Really fixed IMA SDK link.

Fixed again.

Added space

Fixed overvie wheader.

Removed slash file.
@honeybadgerdontcare honeybadgerdontcare requested a review from a user May 15, 2017 15:06
</tr>
<tr>
<td width="40%"><strong>Availability</strong></td>
<td>Experiment</td>
Copy link

Choose a reason for hiding this comment

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

Please change "Experiment" to "Experimental"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

</tr>
<tr>
<td class="col-fourty"><strong><a href="https://www.ampproject.org/docs/guides/responsive/control_layout.html">Supported Layouts</a></strong></td>
<td>fixed, responsive</td>
Copy link

Choose a reason for hiding this comment

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

According to the validator, this component supports: fill, fixed, fixed-height, nodisplay, responsive.

Please correct the list of supported layouts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed that in the validator - I tested the others and they don't work, so I removed them from the validator.

width=640 height=360 layout="responsive"
data-src="https://s0.2mdn.net/4253510/google_ddm_animation_480P.mp4"
data-tag="https://pubads.g.doubleclick.net/gampad/ads?sz=640x480&iu=/124319096/external/ad_rule_samples&ciu_szs=300x250&ad_rule=1&impl=s&gdfp_req=1&env=vp&output=vmap&unviewed_position_start=1&cust_params=deployment%3Ddevsite%26sample_ar%3Dpremidpost&cmsid=496&vid=short_onecue&correlator="
data-poster="path/to/poster.png"
Copy link

Choose a reason for hiding this comment

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

Sample needs a closing bracket (e.g. <)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


## Overview

You can use the `amp-ima-video` component to embed an IMA SDK enabled video
Copy link

Choose a reason for hiding this comment

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

  • Add link to the IMA SDK
  • Rewrite the second sentence (and make it as a paragraph) with these minor tweaks:

To embed a video, provide a source URL for your content video (data-src) and an ad tag (data-tag), which is a URL to a VAST-compliant ad response (for examples, see IMA Sample Tags).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


**data-src** (required)

The URL of your content video.
Copy link

Choose a reason for hiding this comment

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

Either: "The URL of your interactive media ad." or "The URL of your video content."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

<table>
<tr>
<td width="40%"><strong>Description</strong></td>
<td>A video player with an integration of the
Copy link

Choose a reason for hiding this comment

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

Rewrite as:

Embeds a video player for instream video ads that are integrated with the IMA SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

**data-tag** (required)

The URL for your VAST ad document.

Copy link

Choose a reason for hiding this comment

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

Add:

common attributes

This element includes common attributes extended to AMP components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

**data-tag** (required)

The URL for your VAST ad document.

Copy link

Choose a reason for hiding this comment

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

What about the data-poster tag that you have in your sample? Should that be listed as an attribute, like the following:

data-poster (optional)

An image for the frame to be displayed before video playback has started. By default, the first frame is displayed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

<td>Embeds a video player for instream video ads that are integrated with
the
<a href="https://developers.google.com/interactive-media-ads/docs/sdks/html5/">IMA SDK</a>
</td>
Copy link

Choose a reason for hiding this comment

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

nit: Please add period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


You can use the `amp-ima-video` component to embed an <a
href="https://developers.google.com/interactive-media-ads/docs/sdks/html5/">IMA
SDK</a>enabled video player. To embed a video, provide a source URL for your
Copy link

Choose a reason for hiding this comment

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

nit: please add space before "enabled"

Copy link

Choose a reason for hiding this comment

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

Also, please start "To embed" on a new line, that is, as a new paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


This element includes
[common attributes](https://www.ampproject.org/docs/reference/common_attributes)
extended to AMP components
Copy link

Choose a reason for hiding this comment

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

nit: please add period

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

One nit to fix then you can be merged

<td width="40%"><strong>Description</strong></td>
<td>Embeds a video player for instream video ads that are integrated with
the
<a href="https://developers.google.com/interactive-media-ads/docs/sdks/html5/">IMA SDK</a>,
Copy link

Choose a reason for hiding this comment

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

Please change the comma to a period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, sorry :) Fixed.

@shawnbuso
Copy link
Contributor Author

FYI just merged upstream/master into my branch to try to resolve Travis issue.

@ghost ghost merged commit b69d633 into ampproject:master May 29, 2017
@shawnbuso shawnbuso deleted the readme branch July 18, 2017 18:25
This pull request was closed.
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.

None yet

3 participants