Skip to content

Brightcove player component#1052

Merged
dvoytenko merged 1 commit intoampproject:masterfrom
BrightcoveOS:brightcove
Dec 4, 2015
Merged

Brightcove player component#1052
dvoytenko merged 1 commit intoampproject:masterfrom
BrightcoveOS:brightcove

Conversation

@mister-ben
Copy link
Copy Markdown
Contributor

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

Closes #1048.

@googlebot
Copy link
Copy Markdown

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.

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

Comment thread examples/brightcove.amp.html Outdated
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@mister-ben
Copy link
Copy Markdown
Contributor Author

The tests are passing now.

@dvoytenko
Copy link
Copy Markdown
Contributor

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

@mister-ben
Copy link
Copy Markdown
Contributor Author

Squashed commits.

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

Merged. Thanks a lot!

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