✨ Red Bull video player by wklopf · Pull Request #25197 · ampproject/amphtml

wklopf · 2019-10-22T13:39:24Z

Implement Red Bull video player to amp project

amp-owners-bot · 2019-10-22T13:39:34Z

Hey @wassgha, these files were changed:

extensions/amp-redbull-player/0.1/amp-redbull-player.js
extensions/amp-redbull-player/amp-redbull-player.md

Hey @ampproject/wg-caching, these files were changed:

extensions/amp-redbull-player/0.1/test/validator-amp-redbull.html
extensions/amp-redbull-player/0.1/test/validator-amp-redbull.out
extensions/amp-redbull-player/validator-amp-redbull-player.protoascii

googlebot · 2019-10-22T13:40:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

rcebulko

Approving new OWNERS file

lgtm-com · 2019-10-22T14:21:03Z

This pull request introduces 1 alert when merging 27f43e0 into e3cf165 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

newmuis

This change looks like it needs to be properly rebased, as it contains changes to unrelated files (e.g. amp-story-bling-link.css)

gmajoulet · 2019-10-23T18:27:15Z

Thanks for syncing your branch! It still contains a lot of unrelated code, could you please check the "files changed" tab and make sure it only contains your code?
Thanks!

wklopf · 2019-10-24T04:42:03Z

@googlebot I signed it!

…ted unused utils method;

googlebot · 2019-10-24T13:48:52Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

honeybadgerdontcare

Please update the year in all license files to the current year. I approve validation changes.

examples/amp-redbull-player.amp.html

Fazod · 2019-10-24T14:31:46Z

@googlebot I signed it!

googlebot · 2019-10-24T14:31:50Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Fazod · 2019-10-24T15:28:12Z

@googlebot I consent

googlebot · 2019-10-24T15:28:18Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

alanorozco

Thank you for your contribution. Please see comments.

extensions/amp-redbull-player/0.1/amp-redbull-player.js

extensions/amp-redbull-player/0.1/utils/addAttributesToElement.js

extensions/amp-redbull-player/0.1/amp-redbull-player.js

extensions/amp-redbull-player/0.1/test/test-amp-redbull.js

…ed amp submission input;

wklopf · 2019-11-12T13:08:08Z

Hi @alanorozco I updated the files. I hope you find the time to take a look. Thanks!

Fazod · 2019-11-22T07:40:42Z

@alanorozco Hey Alan - is there any chance you could review the changes we did. We here at Red Bull would really appreciate it

wklopf · 2019-12-02T16:09:04Z

Hi everyone, could someone @here give us a hint how we can draw @alanorozco s attention back to this PR? Any suggestion is highly appreciated. Thanks!

honeybadgerdontcare · 2019-12-02T16:11:50Z

There was a United States holiday this past week which may contribute to some delays.

alanorozco · 2019-12-02T19:49:45Z

@wklopf Taking a look, expect a review later today.

alanorozco

One last thing. Thanks!

extensions/amp-redbull-player/0.1/amp-redbull-player.js

wklopf · 2019-12-03T10:45:53Z

Hi @here looks as if we are almost there - exciting!
There is one element left that needs to be reviewed - "percy/amphtml". Please be so kind.

alanorozco · 2019-12-03T16:37:12Z

extensions/amp-redbull-player/0.1/amp-redbull-player.js

+    Services.videoManagerForDoc(this.element).register(this);
+    this.iframe_.contentWindow./*OK*/ postMessage(
+      JSON.stringify(
+        /** @type {!JsonObject} */ ({


Don't cast to JsonObject, use dict().

Using dict() now.

alanorozco · 2019-12-03T16:38:10Z

@wklopf The visual change was a false alarm. I've approved it, but tests will run once again after you resolve the dict() issue I just commented about.

alanorozco

(requesting changes to prevent merge, but there's only that dict() detail left.)

alanorozco · 2019-12-04T16:04:02Z

Thanks @wklopf! Merged. Expect this available on the next canary release.

wklopf · 2019-12-04T16:13:00Z

@here Awesome and thanks to all!

* cl/283618549 Revision bump for #25847 * cl/283654708 mandatory_parent can use spec_name in addition to tag_name * cl/283852039 Introduce a invalid doctype error for amp validation. * cl/283882898 n/a * cl/283993894 Revision bump for #25197 * cl/284115876 Revision bump for #25870 * cl/284258503 Revision bump for #25889 * cl/284856390 Revision bump for #25946

wklopf · 2019-12-17T07:40:52Z

Thanks @wklopf! Merged. Expect this available on the next canary release.

Hi @alanorozco , there have been some releases since then. Any estimate when we will actually make it into a release?

* cl/283618549 Revision bump for ampproject#25847 * cl/283654708 mandatory_parent can use spec_name in addition to tag_name * cl/283852039 Introduce a invalid doctype error for amp validation. * cl/283882898 n/a * cl/283993894 Revision bump for ampproject#25197 * cl/284115876 Revision bump for ampproject#25870 * cl/284258503 Revision bump for ampproject#25889 * cl/284856390 Revision bump for ampproject#25946

googlebot added the cla: no label Oct 22, 2019

rcebulko approved these changes Oct 22, 2019

View reviewed changes

newmuis requested changes Oct 22, 2019

View reviewed changes

wklopf closed this Oct 24, 2019

wklopf reopened this Oct 24, 2019

Add Red Bull video player extension

cf235f1

Fazod force-pushed the master branch from 3cbab23 to cf235f1 Compare October 24, 2019 08:44

(amp-redbull) removed unused import; added jsdoc missing descr.; dele…

10d1508

…ted unused utils method;

(amp-redbull) lint --fix formatted;

f84cf55

honeybadgerdontcare approved these changes Oct 24, 2019

View reviewed changes

examples/amp-redbull-player.amp.html Outdated Show resolved Hide resolved

gmajoulet requested a review from alanorozco October 24, 2019 14:27

(amp-redbull) formatted owners file;

0bff55e

googlebot added cla: yes and removed cla: no labels Oct 24, 2019

alanorozco requested changes Oct 24, 2019

View reviewed changes

alanorozco reviewed Oct 24, 2019

View reviewed changes

extensions/amp-redbull-player/0.1/amp-redbull-player.js Outdated Show resolved Hide resolved

alanorozco reviewed Oct 24, 2019

View reviewed changes

extensions/amp-redbull-player/0.1/amp-redbull-player.js Outdated Show resolved Hide resolved

alanorozco reviewed Oct 24, 2019

View reviewed changes

extensions/amp-redbull-player/0.1/amp-redbull-player.js Outdated Show resolved Hide resolved

alanorozco reviewed Oct 24, 2019

View reviewed changes

extensions/amp-redbull-player/0.1/test/test-amp-redbull.js Outdated Show resolved Hide resolved

extensions/amp-redbull-player/0.1/test/test-amp-redbull.js Show resolved Hide resolved

(amp-redbull) removed unused imports; deleted unused utils; implement…

1b6f6fb

…ed amp submission input;

wklopf added 2 commits November 12, 2019 10:30

(amp-redbull) added skinId to test;

f666abf

(amp-redbull) fixed typo;

9648e7a

wklopf requested a review from alanorozco November 12, 2019 13:47

Gregable added the Validator: Approved label Dec 2, 2019

alanorozco approved these changes Dec 3, 2019

View reviewed changes

extensions/amp-redbull-player/0.1/amp-redbull-player.js Outdated Show resolved Hide resolved

wklopf added 6 commits December 3, 2019 08:32

(amp-redbull) json encoded onReady message;

b787675

(amp-redbull) onReady msg obj updated;

03dd831

(amp-redbull) onReady obj updated;

e9485ad

(amp-redbull) removed onReady message const;

8e40cbd

(amp-redbull) msg obj marked as any;

b58ee07

Merge remote-tracking branch 'upstream/master'

79d5649

alanorozco reviewed Dec 3, 2019

View reviewed changes

alanorozco requested changes Dec 3, 2019

View reviewed changes

(amp-redbull) onReady using dict instead of casting to JsonObj;

db62708

wklopf requested a review from alanorozco December 4, 2019 07:52

alanorozco approved these changes Dec 4, 2019

View reviewed changes

alanorozco merged commit 97d6281 into ampproject:master Dec 4, 2019

micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019

✨ Red Bull video player (ampproject#25197)

d9e2d69

Conversation

wklopf commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amp-owners-bot bot commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Oct 22, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

rcebulko left a comment

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Oct 22, 2019

Uh oh!

newmuis left a comment

Choose a reason for hiding this comment

Uh oh!

gmajoulet commented Oct 23, 2019

Uh oh!

wklopf commented Oct 24, 2019

Uh oh!

googlebot commented Oct 24, 2019

Uh oh!

honeybadgerdontcare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fazod commented Oct 24, 2019

Uh oh!

googlebot commented Oct 24, 2019

Uh oh!

Fazod commented Oct 24, 2019

Uh oh!

googlebot commented Oct 24, 2019

Uh oh!

alanorozco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wklopf commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fazod commented Nov 22, 2019

Uh oh!

wklopf commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

honeybadgerdontcare commented Dec 2, 2019

Uh oh!

alanorozco commented Dec 2, 2019

Uh oh!

alanorozco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wklopf commented Dec 3, 2019

Uh oh!

alanorozco Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

wklopf Dec 4, 2019

Choose a reason for hiding this comment

Uh oh!

wklopf commented Oct 22, 2019 •

edited

Loading

amp-owners-bot bot commented Oct 22, 2019 •

edited

Loading

wklopf commented Nov 12, 2019 •

edited

Loading

wklopf commented Dec 2, 2019 •

edited

Loading