-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
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.
|
|
||
### <a name="amp-brightcove"></a> `amp-brightcove` | ||
|
||
An `amp-brightcove` component displays a Brightcove Video Cloud or Perform player. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a link?
Please add here |
pellentesque, id placerat elit ornare. | ||
</p> | ||
<figure> | ||
<amp-brightcove |
There was a problem hiding this comment.
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.
Everything looks good, but we need to fix CI tests. Please see comments above. Otherwise LGTM on my part. |
CLAs look good, thanks! |
The tests are passing now. |
Awesome. Last request: could you please squash the commits? |
bb8e8b1
to
9a0f608
Compare
Squashed commits. |
LGTM |
Merged. Thanks a lot! |
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! |
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. |
This is an
amp-brightcove
component to implement a Brightcove player as outlined in #1048.Closes #1048.