amphtml icon indicating copy to clipboard operation
amphtml copied to clipboard

new 3q-player feature - a listener for 'resize' event

Open ajvarinho opened this issue 3 years ago • 15 comments

This PR adds a new feature to 3q-player, a listener for 'resize' event. It allows to resize content containers, which is needed for playlists, and it was inspired by already existing AMP method, amp-list. /cc @alanorozco

ajvarinho avatar Feb 08 '22 11:02 ajvarinho

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 08 '22 11:02 CLAassistant

Hey @erwinmombay! These files were changed:

build-system/babel-plugins/OWNERS
build-system/eslint-rules/OWNERS

Hey @estherkim! These files were changed:

build-system/tasks/e2e/index.js

Hey @alanorozco! These files were changed:

build-system/tasks/storybook/index.js

Hey @danielrozenberg! These files were changed:

build-system/tasks/visual-diff/index.js
build-system/tasks/visual-diff/package-lock.json
build-system/tasks/visual-diff/package.json

Hey @ampproject/wg-amp4email! These files were changed:

extensions/amp-accordion/validator-amp-accordion.protoascii
extensions/amp-fit-text/validator-amp-fit-text.protoascii
extensions/amp-lightbox/validator-amp-lightbox.protoascii
extensions/amp-selector/validator-amp-selector.protoascii
extensions/amp-sidebar/validator-amp-sidebar.protoascii

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-accordion/validator-amp-accordion.protoascii
extensions/amp-ad-exit/validator-amp-ad-exit.protoascii
extensions/amp-animation/validator-amp-animation.protoascii
extensions/amp-app-banner/validator-amp-app-banner.protoascii
extensions/amp-audio/validator-amp-audio.protoascii
extensions/amp-brightcove/validator-amp-brightcove.protoascii
extensions/amp-consent/validator-amp-consent.protoascii
extensions/amp-dailymotion/validator-amp-dailymotion.protoascii
extensions/amp-facebook-comments/validator-amp-facebook-comments.protoascii
extensions/amp-facebook-like/validator-amp-facebook-like.protoascii
extensions/amp-facebook-page/validator-amp-facebook-page.protoascii
extensions/amp-facebook/validator-amp-facebook.protoascii
+81 more

Hey @smart-adserver! These files were changed:

extensions/amp-ad-network-smartadserver-impl/0.1/amp-ad-network-smartadserver-impl.js
extensions/amp-ad-network-smartadserver-impl/0.1/test/test-amp-ad-network-smartadserver-impl.js
extensions/amp-ad-network-smartadserver-impl/amp-ad-network-smartadserver-impl-internal.md

Hey @gmajoulet! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.css
extensions/amp-story-360/0.1/amp-story-360.js
extensions/amp-story-interactive/0.1/amp-story-interactive-abstract.js
extensions/amp-story-interactive/0.1/amp-story-interactive-slider.js
extensions/amp-story-interactive/0.1/test/test-amp-story-interactive-slider.js
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-valid.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-valid.out
extensions/amp-story-interactive/validator-amp-story-interactive.protoascii
extensions/amp-story-page-attachment/0.1/amp-story-draggable-drawer.js
extensions/amp-story-shopping/0.1/amp-story-shopping-tag.css
extensions/amp-story-shopping/0.1/amp-story-shopping.md
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-attachment.js
+29 more

Hey @processprocess, @t0mg! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.css
extensions/amp-story-360/0.1/amp-story-360.js

Hey @mszylkowski! These files were changed:

extensions/amp-story-interactive/0.1/amp-story-interactive-abstract.js
extensions/amp-story-interactive/0.1/amp-story-interactive-slider.js
extensions/amp-story-interactive/0.1/test/test-amp-story-interactive-slider.js
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-valid.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-valid.out
extensions/amp-story-interactive/validator-amp-story-interactive.protoascii
extensions/amp-story-page-attachment/0.1/amp-story-draggable-drawer.js

Hey @newmuis! These files were changed:

extensions/amp-story-page-attachment/0.1/amp-story-draggable-drawer.js
extensions/amp-story-subscriptions/0.1/amp-story-subscriptions.css
extensions/amp-story-subscriptions/0.1/amp-story-subscriptions.js
extensions/amp-story-subscriptions/0.1/amp-story-subscriptions.md
extensions/amp-story-subscriptions/0.1/test/test-amp-story-subscriptions.js
extensions/amp-story-subscriptions/0.1/test/validator-amp-story-subscriptions.html
extensions/amp-story-subscriptions/0.1/test/validator-amp-story-subscriptions.out
extensions/amp-story-subscriptions/validator-amp-story-subscriptions.protoascii
extensions/amp-story/1.0/amp-story-desktop-one-panel.css
extensions/amp-story/1.0/amp-story-page.js
extensions/amp-story/1.0/amp-story-store-service.js
extensions/amp-story/1.0/amp-story-system-layer.css
+18 more

Hey @jpettitt! These files were changed:

extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js
extensions/amp-subscriptions-google/0.1/test/test-amp-subscriptions-google.js
extensions/amp-subscriptions-google/OWNERS
extensions/amp-subscriptions/OWNERS
extensions/amp-subscriptions/validator-amp-subscriptions.protoascii
third_party/subscriptions-project/OWNERS
third_party/subscriptions-project/config.js
third_party/subscriptions-project/swg-button.css
third_party/subscriptions-project/swg-gaa.js
third_party/subscriptions-project/swg.js

amp-owners-bot[bot] avatar Feb 08 '22 11:02 amp-owners-bot[bot]

I've just signed the CLA

Best regards

andreasschrottenbaum avatar Feb 08 '22 15:02 andreasschrottenbaum

@andreasschrottenbaum Unsure why the CLA check is still failing. It's likely that @aywar2000 needs to sign as well.

alanorozco avatar Feb 08 '22 17:02 alanorozco

I've just signed the CLA

Best regards

ajvarinho avatar Feb 09 '22 09:02 ajvarinho

Hey Alan, thanks for your time and reviews. I added the catch block.

ajvarinho avatar Feb 11 '22 11:02 ajvarinho

Still getting formatting issues https://app.circleci.com/pipelines/github/ampproject/amphtml/22925/workflows/f0712bcc-0c5d-4127-add5-f59e63dd695e/jobs/457826

alanorozco avatar Feb 15 '22 16:02 alanorozco

Dear Alan, thanks again for your time, I fixed the formatting issues. @danielrozenberg @rebeccanthomas, could you please review it, so we can merge it this week? Thanks!

ajvarinho avatar Feb 21 '22 11:02 ajvarinho

Dear Alan, thanks again for your time, I fixed the formatting issues. @danielrozenberg @rebeccanthomas, could you please review it, so we can merge it this week? Thanks!

It looks like Rebecca and I were auto-added to this PR for an earlier revision (probably one with a large merge commit?) and it doesn't seem like any of the files that either of us are responsible for are being changed here. Alan's review should be enough here :)

danielrozenberg avatar Feb 22 '22 16:02 danielrozenberg

It looks like the validator file was removed, so approval from "WG: Caching" is no longer needed.

Gregable avatar Feb 22 '22 23:02 Gregable

Hey @danielrozenberg , hey @Gregable ; thanks for your time and for looking into the matter. It looks like I did not built AMP completely, which is why I had formatting errors on reviews. @alanorozco, I apologize. Will update the pull request tomorrow, so we can merge it.

ajvarinho avatar Feb 23 '22 21:02 ajvarinho

@aywar2000 Apologies, CI is failing for unrelated reasons. Push an empty commit to re-run:

git commit --allow-empty -m "empty commit to trigger ci"

alanorozco avatar Apr 14 '22 15:04 alanorozco

Hey @alanorozco, thanks for getting back. I've just pushed an empty commit.

ajvarinho avatar Apr 20 '22 07:04 ajvarinho

Dear @alanorozco, I've just commited an unrelated fix to my amphtml repository that really should be merged as soon as possible. Could you please look into it?

ajvarinho avatar May 02 '22 15:05 ajvarinho

I've just signed the CLA

Best regards

ajvarinho avatar May 03 '22 12:05 ajvarinho

I've just signed the CLA

Best regards

ajvarinho avatar Oct 27 '22 08:10 ajvarinho

I've just signed the CLA

Best regards

ajvarinho avatar Nov 08 '22 16:11 ajvarinho

I've just signed the CLA

Best regards

ajvarinho avatar Nov 09 '22 09:11 ajvarinho

Hello @danielrozenberg, can we please check the owners approvals and CLA license? Thanks for your time:)

ajvarinho avatar Nov 14 '22 14:11 ajvarinho

It looks like you've added a lot of commits to this branch by a user that's not associated with a GitHub account (I assume that's you, just with commits signed using a different email address). My recommendation is to squash your branch* into a single commit, and making sure you use the same email address that you use on GitHub to sign these commits

*note that we use main as the name our main branch, not master

danielrozenberg avatar Nov 14 '22 20:11 danielrozenberg

It looks like you've added a lot of commits to this branch by a user that's not associated with a GitHub account (I assume that's you, just with commits signed using a different email address). My recommendation is to squash your branch* into a single commit, and making sure you use the same email address that you use on GitHub to sign these commits

*note that we use main as the name our main branch, not master

Thanks for the reply, will try your recommendation.

ajvarinho avatar Nov 17 '22 14:11 ajvarinho

Hey @danielrozenberg, can you please help out? This is stalled here for ages, tried the slack channel, tried Stack Overflow, tried to write your colleagues from AMP, asked my senior colleagues in the company and this thing is not moving anywhere. What should I do, please?

ajvarinho avatar Dec 09 '22 08:12 ajvarinho

It looks like you're changing package.json and package-lock.json unrelatedly to this PR, if you revert these files it would make it so that you won't need my approval. Perhaps @alanorozco can help with getting approval from the caching team for the other file

danielrozenberg avatar Dec 09 '22 19:12 danielrozenberg

Hey @alanorozco @danielrozenberg squashed all the commits as advised, which resulted in about 315 files changed, and some of it were validator- files. I had to change one validator file to get the PR to pass CircleCi check. As i pulled changes from AMP branch, it caused changes to package- files, including .json. If I try to revert now, there are merge conflicts for 315 files. If I try to solve merge conflicts in the reverted commit, I have to do the npm install to run tests. I can't run tests because I have to have package.json updated to run npm install, and if I update the package.json so I can run npm install, I can't revert the commit and therefore the problems persist.

Would it make sense just to move away from all this mess and to open a new PR request?

ajvarinho avatar Dec 12 '22 10:12 ajvarinho

Perhaps opening a new PR will make things easier. Make sure you work on a branch with another name (not main) so it'll be easy to merge changes from this repository and your own fork

danielrozenberg avatar Dec 12 '22 17:12 danielrozenberg