Skip to content

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

Merged
7 commits merged intoampproject:masterfrom
shawnbuso:readme
May 29, 2017
Merged

amp-ima-video: Markdown README for project#9336
7 commits merged intoampproject:masterfrom
shawnbuso:readme

Conversation

@shawnbuso
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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.

3 participants