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

Brightcove player component #1052

Merged
merged 1 commit into from
Dec 4, 2015
Merged

Conversation

mister-ben
Copy link
Contributor

This is an amp-brightcove component to implement a Brightcove player as outlined in #1048.

Closes #1048.

@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 you signed the CLA as a corporation, please let us know the company's name.


### <a name="amp-brightcove"></a> `amp-brightcove`

An `amp-brightcove` component displays a Brightcove Video Cloud or Perform player.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a link?

@cramforce
Copy link
Member

Please add here
https://github.com/ampproject/amphtml/blob/master/test/integration/test-example-validation.js#L39
and whitelist the failure for now (because the validator was not yet updated)

pellentesque, id placerat elit ornare.
</p>
<figure>
<amp-brightcove
Copy link
Member

Choose a reason for hiding this comment

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

Up to you but a simpler or more focused example might be easier to comprehend.

@dvoytenko
Copy link
Contributor

Everything looks good, but we need to fix CI tests. Please see comments above. Otherwise LGTM on my part.

@googlebot
Copy link

CLAs look good, thanks!

@mister-ben
Copy link
Contributor Author

The tests are passing now.

@dvoytenko
Copy link
Contributor

Awesome. Last request: could you please squash the commits?

@mister-ben
Copy link
Contributor Author

Squashed commits.

@dvoytenko
Copy link
Contributor

LGTM

dvoytenko added a commit that referenced this pull request Dec 4, 2015
@dvoytenko dvoytenko merged commit 175db5e into ampproject:master Dec 4, 2015
@dvoytenko
Copy link
Contributor

Merged. Thanks a lot!

@jasti
Copy link
Contributor

jasti commented Feb 8, 2016

hi @mister-ben , would you know if this PR would work for publishers that use the 'old' Brightcove player? Sorry about the the vagueness here but I have a publisher asking the question. (I don't know if some pubs still use the old brightcove player for specific features which might not be available when implementing the amp-brightcove component?) Thanks!

@mister-ben
Copy link
Contributor Author

Hi @jasti . This is for the new Brightcove Player, not the "Smart Player"/old player. The two versions have entirely different embed codes and underlying technologies. However publishers can use different both versions concurrently; that they're using the Smart Player on their main pages doesn't necessarily stop them from the Brightcove Player in AMP. If there is a blocking issue it would be good to understand that.

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.

5 participants