(`:fire:`) AMP MowPlayer - Responsive Feature #19721

gopanisandip · 2018-12-07T16:33:29Z

I have added functionality to make our simple player and player with playlist responsive.

…ayer-new

…l upstream master

…ayer-new

torch2424 · 2018-12-08T00:13:25Z

Traiging to @alanorozco since it involve amp-video 😄 Feel free to re-assign

torch2424

Will let @alanorozco do the review, but noticed something 😄

torch2424 · 2018-12-08T00:14:09Z

extensions/amp-mowplayer/OWNERS.yaml

@@ -1 +1,5 @@
+<<<<<<< HEAD


We should remove merge conflict issues 👍

Hi i have resolved conflicts. Could you do more review? so that we can more forward.

Hello, thank you for fixing the merge conflicts!

I'll let @aghassemi and @alanorozco handle the rest of the review, as they are much more familiar than I am with the video players in AMP. Thank you for your contribution however! 😄

…ayer-new

aghassemi · 2018-12-10T17:57:54Z

extensions/amp-mowplayer/0.1/amp-mowplayer.js

@@ -228,6 +230,10 @@ class AmpMowplayer extends AMP.BaseElement {

    const {element} = this;

+    if (eventType === 'set_aspect_ratio') {


This equates to a height change which is not always allowed. attemptChangeHeight is the API to use here.

@gopanisandip could you explain what this PR is trying to do?

It looks like this is doing something very similar to layout=responsive? padding-top hack and all :)

@alanorozco @aghassemi Let me explain what we need is : We have player with playlist so in that case aspect ration of player is not always 16:9 because we have feature in mowplayer.com so that user can choose position of playlist like right side of player and bottom of player. so in that case we have 2 different aspect ratio. Also if user choose right position of playlist than also we have it in bottom in mobile device.

so when ever player load or screen rotate at that time we event will be fired from iframe with new aspect ratio and in this way we can make it responsive. This event will calculate padding-Top based on internal content of player.

This is not limited to playlist only we have another version of player which have different aspect ration based on screen resolution aspect ration will change.

Please check this screenshot links :

http://prntscr.com/ltbzev
http://prntscr.com/ltbzpq
http://prntscr.com/ltbzu3
http://prntscr.com/ltbzxw
http://prntscr.com/ltc030

Could help me to figure out this? Problem here is we don't have fixed aspect ratio.

Got it. See @aghassemi's comment above. You need to calculate the fixed height based on the component's width (basically height = width * scale, where scale is the value received as padding-top divided by 100).

Then you can attemptChangeHeight(height). The runtime may reject the request to change height, and that would be working as intended (It's one of AMP's core promises that content should not shift vertically under certain conditions).

Got it. Thanks will change it!

…ayer-new

gopanisandip · 2018-12-21T09:01:31Z

@alanorozco Could you please take a look?

aghassemi · 2018-12-21T16:09:03Z

@gopanisandip Changes look good but there are couple of unrelated files checked in (iltorb@2.4.1, node) and a unrelated change. Please revert those. Thanks.

aghassemi · 2018-12-21T16:07:52Z

validator/engine/validator.js

@@ -4130,7 +4130,7 @@ function validateAttributes(
      }
    }
  }
-  let missingAttrs = [];
+  const missingAttrs = [];


please revert this

gopanisandip · 2018-12-24T03:33:39Z

@alanorozco Could you please check now. i reverted validator/engine/validator.js file. I had used following commands to revert file and take latest pull.

git checkout validator/engine/validator.js
git add validator/engine/validator.js
git commit -m ""
git pull upstream master
git push

Let me know if still any changes are required.

aghassemi · 2018-12-24T15:36:09Z

@gopanisandip please see the "changes" tab of this PR, there are still u related files checked in

…ayer-new

This reverts commit aebd8ea.

…ayer-new

gopanisandip · 2018-12-24T16:18:41Z

@alanorozco @aghassemi I had removed all in related files changes. Could you please take a look?

aghassemi · 2018-12-24T20:02:10Z

extensions/amp-mowplayer/OWNERS.yaml

@@ -1 +1 @@
- gopanisandip
+gopanisandip


- should not be removed from this file.

Ok this is reverted. Ideally there should not be any change on this file but due to conflicts previously we have this file got updated.

Anyway, Could you please take a look again in this PR and let me know if any changes required.

gopanisandip · 2018-12-25T05:16:44Z

@aghassemi So whats next?

aghassemi · 2018-12-25T06:23:06Z

@gopanisandip Lint is failing on the CI, please run gulp lint locally and fix the issue, after that CI checks should be green and I will merge the PR

…ayer-new

gopanisandip · 2018-12-31T07:18:00Z

@aghassemi could you please move forward with merging?

* Added code for amp-mowplayer extension * Fixed lint errors * Removed satisfies/requires pairs * Fixed link errors and removed unused method * Added JsonObject type casting * Added json typecast * Added username in OWNERS.yaml * supported layout in validator files needed by match IsFixedSize * autoplay attribute was missing * Update validator-amp-mowplayer.protoascii * Update validator-amp-mowplayer.protoascii * Update validator-amp-mowplayer.protoascii * Regenerated .out file * added resposive feature via postMessage * Fixed lint errors * Removed Conflicts * Resolved conflicts in owners.yml * Added attemptHeightChange * Revert "Added attemptHeightChange" This reverts commit aebd8ea. * Reveted in expected change and added attemtheightchange * Revert unwanted changes * Revert OWNERS.yaml * Changes in parameter * Fixed Lint errors and check type

gopanisandip and others added 22 commits August 6, 2018 11:57

Added code for amp-mowplayer extension

1421a7f

Merge branch 'master' into amp-mowplayer-new

5c4d76b

Fixed lint errors

56fdb8e

Removed satisfies/requires pairs

8fb5cb3

Fixed link errors and removed unused method

32d7bb7

Added JsonObject type casting

2d39d1f

Added json typecast

e45a224

Merge branch 'master' of github.com:ampproject/amphtml into amp-mowpl…

ab90fbe

…ayer-new

Added username in OWNERS.yaml

e228de2

supported layout in validator files needed by match IsFixedSize

4b56dad

autoplay attribute was missing

bc70c17

Update validator-amp-mowplayer.protoascii

a29a35a

Update validator-amp-mowplayer.protoascii

2f82d81

Update validator-amp-mowplayer.protoascii

6f179f5

Merge branch 'master' into amp-mowplayer-new

93e2f38

Regenerated .out file

56682eb

Resolved conflict in dep-check-config.js while runnig command git pul…

9160ed8

…l upstream master

Resolved conflicts

a8bda31

Merge branch 'master' of github.com:ampproject/amphtml into amp-mowpl…

fe1abab

…ayer-new

added resposive feature via postMessage

d69db4a

Fixed lint errors

45409b6

Resolved Conflicts

c7dd0cb

googlebot added the cla: yes label Dec 7, 2018

gopanisandip added 2 commits December 7, 2018 22:05

Removed Conflicts

07e2b36

Merge branch 'master' of github.com:ampproject/amphtml into amp-mowpl…

71c186c

…ayer-new

torch2424 requested a review from alanorozco December 8, 2018 00:13

torch2424 reviewed Dec 8, 2018

View reviewed changes

gopanisandip added 2 commits December 8, 2018 09:02

Resolved conflicts in owners.yml

562314b

Merge branch 'master' of github.com:ampproject/amphtml into amp-mowpl…

c2b9189

…ayer-new

aghassemi suggested changes Dec 10, 2018

View reviewed changes

gopanisandip added 3 commits December 20, 2018 15:51

Merge branch 'master' of github.com:ampproject/amphtml into amp-mowpl…

f409aad

…ayer-new

Added attemptHeightChange

aebd8ea

Merge branch 'master' of github.com:ampproject/amphtml into amp-mowpl…

03b179e

…ayer-new

aghassemi reviewed Dec 21, 2018

View reviewed changes

Merge branch 'master' of github.com:ampproject/amphtml into amp-mowpl…

cc14750

…ayer-new

gopanisandip force-pushed the amp-mowplayer-new branch from ac95c28 to cc14750 Compare December 24, 2018 15:54

gopanisandip added 3 commits December 24, 2018 21:34

Revert "Added attemptHeightChange"

0595d89

This reverts commit aebd8ea.

Merge branch 'master' of github.com:ampproject/amphtml into amp-mowpl…

8d43c96

…ayer-new

Reveted in expected change and added attemtheightchange

6e188d1

aghassemi reviewed Dec 24, 2018

View reviewed changes

gopanisandip added 2 commits December 25, 2018 09:46

Revert unwanted changes

ac38a04

Revert OWNERS.yaml

1d1d85e

aghassemi approved these changes Dec 25, 2018

View reviewed changes

gopanisandip added 3 commits December 31, 2018 11:12

Changes in parameter

9e95b4f

Merge branch 'master' of github.com:ampproject/amphtml into amp-mowpl…

1d6b3c8

…ayer-new

Fixed Lint errors and check type

ba5e8cb

aghassemi approved these changes Dec 31, 2018

View reviewed changes

aghassemi merged commit bfe38f2 into ampproject:master Dec 31, 2018

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Jan 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(`:fire:`) AMP MowPlayer - Responsive Feature #19721

(`:fire:`) AMP MowPlayer - Responsive Feature #19721

gopanisandip commented Dec 7, 2018

torch2424 commented Dec 8, 2018

torch2424 left a comment

torch2424 Dec 8, 2018

gopanisandip Dec 8, 2018

torch2424 Dec 10, 2018

aghassemi Dec 10, 2018 •

edited

alanorozco Dec 10, 2018

gopanisandip Dec 11, 2018

alanorozco Dec 11, 2018 •

edited

gopanisandip Dec 12, 2018

gopanisandip commented Dec 21, 2018

aghassemi commented Dec 21, 2018

aghassemi Dec 21, 2018

gopanisandip commented Dec 24, 2018

aghassemi commented Dec 24, 2018

gopanisandip commented Dec 24, 2018 •

edited

aghassemi Dec 24, 2018

gopanisandip Dec 25, 2018

gopanisandip commented Dec 25, 2018

aghassemi commented Dec 25, 2018

gopanisandip commented Dec 31, 2018

		@@ -228,6 +230,10 @@ class AmpMowplayer extends AMP.BaseElement {

		const {element} = this;

		if (eventType === 'set_aspect_ratio') {

(:fire:) AMP MowPlayer - Responsive Feature #19721

(:fire:) AMP MowPlayer - Responsive Feature #19721

Conversation

gopanisandip commented Dec 7, 2018

torch2424 commented Dec 8, 2018

torch2424 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi Dec 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanorozco Dec 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gopanisandip commented Dec 21, 2018

aghassemi commented Dec 21, 2018

Choose a reason for hiding this comment

gopanisandip commented Dec 24, 2018

aghassemi commented Dec 24, 2018

gopanisandip commented Dec 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gopanisandip commented Dec 25, 2018

aghassemi commented Dec 25, 2018

gopanisandip commented Dec 31, 2018

(`:fire:`) AMP MowPlayer - Responsive Feature #19721

(`:fire:`) AMP MowPlayer - Responsive Feature #19721

aghassemi Dec 10, 2018 •

edited

alanorozco Dec 11, 2018 •

edited

gopanisandip commented Dec 24, 2018 •

edited