AMP Delight Player by xymw · Pull Request #17939 · ampproject/amphtml

xymw · 2018-09-07T13:50:40Z

This is the amp-delight-player component from the intent to implement issue: #17772

Thank you,
the Delight Team

- renamed from 'amp-delight' to 'amp-delight-player' - implemented getCurrentTime interface - implemented getDuration interface - implemented getPlayedRanges interface

- implemented dock behavior - implemented autoplay behavior - implemented showControls interface - implemented hideControls interface - refined fullscreen handling - refined iframe event system - testing and validation - updated docs

googlebot · 2018-09-07T13:50:43Z

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 (e.g. 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.

ghost · 2018-09-07T14:18:46Z

This pull request introduces 1 alert when merging c87e7e8 into c2a7a19 - view on LGTM.com

new alerts:

1 for Identical operands

Comment posted by LGTM.com

xymw · 2018-09-07T14:20:25Z

I signed it!

- fixed linting issues - improved and simplified iframe event communication

xymw · 2018-09-13T10:27:29Z

I signed the CLA.

googlebot · 2018-09-13T10:27:36Z

CLAs look good, thanks!

- added amp-delight-player.js to whitelist for video-manager-impl.js service - fixed use of postMessage

xymw · 2018-09-21T07:18:25Z

Travis bundle-size check is throwing errors because the size of v0.js has increased by 0.08KB, most likely due to the addition of amp-delight-player in bundles.config.js. How can this be resolved @alanorozco @choumx?

FAIL  ./dist/v0.js: 81.68KB > maxSize 81.6KB (gzip) 
ERROR: bundlesize found that ./dist/v0.js has exceeded its size cap of 81.6KB.
This is part of a new effort to reduce AMP's binary size (#14392).
Please contact @choumx or @jridgewell for assistance.

dreamofabear · 2018-09-21T14:33:32Z

That shouldn't increase v0.js size. I think your branch point had a breakage -- try merging latest master.

alanorozco

Thanks for your contribution. Made a few change requests.

extensions/amp-delight-player/0.1/amp-delight-player.css

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

- fixed css formatting - use i-amphtml-faded class instead of faded - set more style properties in css instead of js - simplified firstLayoutCompleted() - removed unnecessary super calls in pauseCallback and resumeCallback - use listen() util instead of raw addEventListener

xymw · 2018-10-02T10:01:23Z

Made the requested changes and all checks passed. Waiting for further input.

alanorozco · 2018-10-02T18:13:20Z

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

+    this.element.classList.add('i-amphtml-expanded');
+    let viewportMeta = document.querySelector('#dl8-meta-viewport');
+    if (!viewportMeta) {
+      viewportMeta = document.createElement('meta');


This shouldn't be necessary as it's part of the AMP boilerplate.

Gregable

Some comments on the validator rules only.

Gregable · 2018-10-04T01:55:55Z

extensions/amp-delight-player/validator-amp-delight-player.protoascii

+    name: "amp-delight-player"
+    version: "0.1"
+    version: "latest"
+    requires_usage: GRANDFATHERED


Please remove this one line. All new extensions require usage to be added to the document.

Gregable · 2018-10-04T01:57:15Z

extensions/amp-delight-player/validator-amp-delight-player.protoascii

+    version: "0.1"
+    version: "latest"
+    requires_usage: GRANDFATHERED
+    deprecated_allow_duplicates: true


Please remove this one line. All new extensions require that multiple copies may not be included in the same page.

aghassemi · 2018-10-08T22:46:01Z

@xymw thanks for contributing this component. Just a friendly ping for the last couple of remaining code review requests and then we can merge.

- removed unnecessary viewport manipulation - removed validator rules

aghassemi · 2018-10-09T17:17:03Z

Thanks @xymw! @alanorozco @Gregable PTAL.

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

xymw · 2018-10-29T12:01:48Z

Hey @alanorozco @Gregable @aghassemi what is the status of the PR? Do we need to implement any further changes before merge or is everything good to go?

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

extensions/amp-delight-player/amp-delight-player.md

- removed unused redirect iframe event - use attemptChangeHeight to change height - use template strings - removed event section in amp-delight-player.md

aghassemi

@xymw Just a few more requests and we can merge after. Thanks for implementing all these features for the AMP version of your video player and writing great code! Great PR overall.

extensions/amp-delight-player/amp-delight-player.md

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

extensions/amp-delight-player/0.1/amp-delight-player.css

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

- renamed i-amphtml-faded css class into i-amphtml-delight-player-faded - removed unused i-amphtml-expanded - removed use of actionServiceForDoc action triggering - fixed up amp-delight-player.md

aghassemi · 2018-11-12T21:21:56Z

Thanks @xymw

alin04 · 2018-11-13T00:12:36Z

extensions/amp-delight-player/0.1/test/validator-amp-delight.player.out

@@ -0,0 +1,46 @@
+FAIL


This file looks like extraneous? Mistyped filename?

* cl/220306609 Revision bump for #17907 * cl/220307253 Revision bump for #19128 * cl/220310523 Revision bump for #19129 * cl/220399983 Revision bump for #19167 * cl/221145203 n/a * cl/221159765 Revision bump for #19214 * cl/221164382 Invalidate `<amp-script>` tag as well * cl/221176616 Revision bump for #17939 * cl/221181356 Revision bump for #19171

* cl/220306609 Revision bump for ampproject#17907 * cl/220307253 Revision bump for ampproject#19128 * cl/220310523 Revision bump for ampproject#19129 * cl/220399983 Revision bump for ampproject#19167 * cl/221145203 n/a * cl/221159765 Revision bump for ampproject#19214 * cl/221164382 Invalidate `<amp-script>` tag as well * cl/221176616 Revision bump for ampproject#17939 * cl/221181356 Revision bump for ampproject#19171

Matthias Wolff added 3 commits August 29, 2018 18:57

amp-delight extension implementation

9d7d78d

Renamed element and extended video interface

68b8835

- renamed from 'amp-delight' to 'amp-delight-player' - implemented getCurrentTime interface - implemented getDuration interface - implemented getPlayedRanges interface

prepare and polish for PR

c87e7e8

- implemented dock behavior - implemented autoplay behavior - implemented showControls interface - implemented hideControls interface - refined fullscreen handling - refined iframe event system - testing and validation - updated docs

googlebot added the cla: no label Sep 7, 2018

remove unnecessary attribute check

28191a4

aghassemi requested a review from alanorozco September 7, 2018 17:31

improved iframe events and formating fixes

6dc1d56

- fixed linting issues - improved and simplified iframe event communication

googlebot added cla: yes and removed cla: no labels Sep 13, 2018

fr- and others added 9 commits September 13, 2018 18:05

Remove trailing ws and add commacomma-dangle

faa4f2f

fixed travis ci issues

410d100

- added amp-delight-player.js to whitelist for video-manager-impl.js service - fixed use of postMessage

fixed JSDoc annotations and other linting errors

c993588

removed unnecessary modification of display prop

d559a9b

removed unnecessary modification of display (2)

ebabc41

fixed tests and added delight-player to readme

be203be

fixed event triggering, fixed linting

7833835

fixed tests

03a9b16

Merge remote-tracking branch 'old/master' into amp-delight

5dfea45

Merge remote-tracking branch 'origin/master' into amp-delight

b6fe7c0

alanorozco requested changes Sep 21, 2018

View reviewed changes

Matthias Wolff added 3 commits September 24, 2018 12:14

fixed test

77eecad

Merge remote-tracking branch 'old/master' into amp-delight

cc10672

alanorozco requested changes Oct 2, 2018

View reviewed changes

Gregable requested changes Oct 4, 2018

View reviewed changes

Matthias Wolff added 2 commits October 9, 2018 09:48

implemented change requests

45fca39

- removed unnecessary viewport manipulation - removed validator rules

Merge remote-tracking branch 'old/master' into amp-delight

5203e2d

alanorozco approved these changes Oct 11, 2018

View reviewed changes

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

Gregable approved these changes Oct 29, 2018

View reviewed changes

aghassemi suggested changes Oct 29, 2018

View reviewed changes

Matthias Wolff added 3 commits November 2, 2018 14:04

code review changes

fbc62be

- removed unused redirect iframe event - use attemptChangeHeight to change height - use template strings - removed event section in amp-delight-player.md

Merge remote-tracking branch 'old/master' into amp-delight

5215870

simplified technique to expand iframe to full height

c9312c4

aghassemi suggested changes Nov 2, 2018

View reviewed changes

Matthias Wolff added 2 commits November 7, 2018 10:10

pr change requests

2c6a22f

- renamed i-amphtml-faded css class into i-amphtml-delight-player-faded - removed unused i-amphtml-expanded - removed use of actionServiceForDoc action triggering - fixed up amp-delight-player.md

Merge remote-tracking branch 'old/master' into amp-delight

9f804ad

aghassemi approved these changes Nov 12, 2018

View reviewed changes

aghassemi merged commit fe4d169 into ampproject:master Nov 12, 2018

alin04 reviewed Nov 13, 2018

View reviewed changes

alin04 added a commit to alin04/amphtml that referenced this pull request Nov 13, 2018

cl/221176616 Revision bump for ampproject#17939

41ecdf0

alin04 mentioned this pull request Nov 13, 2018

Validator rollup #19294

Merged

ampprojectbot added the PR use: In Beta / Experimental label Nov 14, 2018

Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018

✨ amp-delight-player (ampproject#17939)

8e2bb30

Conversation

xymw commented Sep 7, 2018

Uh oh!

googlebot commented Sep 7, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

ghost commented Sep 7, 2018

Uh oh!

xymw commented Sep 7, 2018

Uh oh!

xymw commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Sep 13, 2018

Uh oh!

xymw commented Sep 21, 2018

Uh oh!

dreamofabear commented Sep 21, 2018

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!

xymw commented Oct 2, 2018

Uh oh!

alanorozco Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

Gregable left a comment

Choose a reason for hiding this comment

Uh oh!

Gregable Oct 4, 2018

Choose a reason for hiding this comment

Uh oh!

Gregable Oct 4, 2018

Choose a reason for hiding this comment

Uh oh!

aghassemi commented Oct 8, 2018

Uh oh!

aghassemi commented Oct 9, 2018

Uh oh!

Uh oh!

xymw commented Oct 29, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aghassemi 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!

aghassemi commented Nov 12, 2018

Uh oh!

alin04 Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

xymw commented Sep 13, 2018 •

edited

Loading