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

Conversation

@mister-ben
Copy link
Contributor

commented Dec 2, 2015

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

Closes #1048.

@googlebot

This comment has been minimized.

Copy link

commented Dec 2, 2015

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.
@cramforce

View changes

extensions/amp-brightcove/amp-brightcove.md Outdated

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

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

This comment has been minimized.

Copy link
@cramforce

cramforce Dec 3, 2015

Member

Maybe add a link?

@cramforce

This comment has been minimized.

Copy link
Member

commented Dec 3, 2015

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)

@cramforce

View changes

examples/brightcove.amp.html Outdated
pellentesque, id placerat elit ornare.
</p>
<figure>
<amp-brightcove

This comment has been minimized.

Copy link
@cramforce

cramforce Dec 3, 2015

Member

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

@dvoytenko

View changes

examples/brightcove.amp.html Outdated
<main role="main">
<article>

<amp-lightbox id="headline-img-lightbox" class="lightbox"

This comment has been minimized.

Copy link
@dvoytenko

dvoytenko Dec 3, 2015

Collaborator

This is not needed for our specific purpose here, right?

@dvoytenko

View changes

extensions/amp-brightcove/0.1/amp-brightcove.js Outdated
var playerid = (this.element.getAttribute('data-player-id') || 'default');
var embed = (this.element.getAttribute('data-embed') || 'default');
var iframe = document.createElement('iframe');
var src = 'https://players.brightcove.net/' + account + '/' + playerid + '_' + embed + '/index.html';

This comment has been minimized.

Copy link
@dvoytenko

dvoytenko Dec 3, 2015

Collaborator

Please use encodeURIComponent for all variable parts of the URL.

@dvoytenko

View changes

gulpfile.js Outdated
@@ -86,6 +86,7 @@ function buildExtensions(options) {
buildExtension('amp-twitter', '0.1', false, options);
buildExtension('amp-vine', '0.1', false, options);
buildExtension('amp-youtube', '0.1', false, options);
buildExtension('amp-brightcove', '0.1', false, options);

This comment has been minimized.

Copy link
@dvoytenko

dvoytenko Dec 3, 2015

Collaborator

Please sort alphabetically.

this.element.appendChild(iframe);
/** @private {?Element} */
this.iframe_ = iframe;
return loadPromise(iframe);

This comment has been minimized.

Copy link
@dvoytenko

dvoytenko Dec 3, 2015

Collaborator

Please also whitelist your element in the https://github.com/ampproject/amphtml/blob/master/src/layout.js#L73 - it's a minor thing, but a good touch.

limitations under the License.
-->

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

This comment has been minimized.

Copy link
@dvoytenko
@dvoytenko

View changes

extensions/amp-brightcove/0.1/amp-brightcove.js Outdated

/** @private */
encodeId_(id) {
if (id.substring(0,4) === 'ref:') {

This comment has been minimized.

Copy link
@dvoytenko

dvoytenko Dec 3, 2015

Collaborator

Could you please add a comment explaining why?

@dvoytenko

View changes

test/integration/test-example-validation.js Outdated
@@ -67,6 +68,9 @@ describe('example', function() {

// TODO(dvoytenko): Remove once validator supports "amp-font" element.
/DISALLOWED_TAG amp-font/,

// Remove once validator supports "data-account" for "amp-brightcove".
/MANDATORY_ATTR_MISSING account/

This comment has been minimized.

Copy link
@dvoytenko

dvoytenko Dec 3, 2015

Collaborator

I believe this should be instead:

/DISALLOWED_TAG amp-brightcove/,

Data-attributes are all whitelisted by default. But our validator doesn't know anything about "amp-brightcove" element yet - hence whitelist.

This comment has been minimized.

Copy link
@mister-ben

mister-ben Dec 3, 2015

Author Contributor

Ah right, got it.

@dvoytenko

View changes

extensions/amp-brightcove/0.1/test/test-amp-brightcove.js Outdated

it('renders', () => {
return getBrightcove('906043040001').then((yt) => {
var iframe = bc.querySelector('iframe');

This comment has been minimized.

Copy link
@dvoytenko

dvoytenko Dec 3, 2015

Collaborator

I don't see bc variable anywhere in this scope.

@dvoytenko

View changes

extensions/amp-brightcove/0.1/amp-brightcove.js Outdated

/** @override */
layoutCallback() {
var width = this.element.getAttribute('width');

This comment has been minimized.

Copy link
@dvoytenko

dvoytenko Dec 3, 2015

Collaborator

Our linter will disallow all of these. Could you please to let and const? You can run gulp lint - it will report all of these cases at once.

@dvoytenko

View changes

extensions/amp-brightcove/0.1/test/test-amp-brightcove.js Outdated

function getBrightcove(accountId, videoId, opt_responsive) {
return createIframePromise().then((iframe) => {
var bc = iframe.doc.createElement('amp-brightcove');

This comment has been minimized.

Copy link
@dvoytenko

dvoytenko Dec 3, 2015

Collaborator

Ditto on let and const.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Dec 3, 2015

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

@googlebot

This comment has been minimized.

Copy link

commented Dec 3, 2015

CLAs look good, thanks!

@mister-ben

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2015

The tests are passing now.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Dec 3, 2015

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

@mister-ben mister-ben force-pushed the BrightcoveOS:brightcove branch to 9a0f608 Dec 3, 2015

@mister-ben

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2015

Squashed commits.

@dvoytenko dvoytenko added LGTM and removed NEEDS REVIEW labels Dec 4, 2015

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Dec 4, 2015

LGTM

dvoytenko added a commit that referenced this pull request Dec 4, 2015
Merge pull request #1052 from BrightcoveOS/brightcove
Brightcove player component

@dvoytenko dvoytenko merged commit 175db5e into ampproject:master Dec 4, 2015

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Dec 4, 2015

Merged. Thanks a lot!

@jasti

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2016

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
Projects
None yet
5 participants
You can’t perform that action at this time.