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.

Sign up for GitHub

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

Jump to bottom

AMP Video Players: use IntersectionObserver instead of viewportCallback + scroll detection. #30647

Merged
merged 3 commits into from
Oct 27, 2020
Merged

AMP Video Players: use IntersectionObserver instead of viewportCallback + scroll detection. #30647

merged 3 commits into from
Oct 27, 2020

Conversation

samouri
Copy link
Member

@samouri samouri commented Oct 13, 2020 •
edited

summary
Supersedes #30619. A more aggressive take on the same idea.
Partial for: #30620.
Fixes #30818
Blocked by #30761

TODO:

  • manual testing
  • update unit tests
  • feature detection

@google-cla google-cla bot added the cla: yes label Oct 13, 2020
@samouri samouri force-pushed the remove-video-vpcb branch 2 times, most recently from 8f71431 to 8cf8844 Compare October 13, 2020 21:13
@samouri samouri changed the title amp-video: remove all traces of viewportCallback AMP Video Players: use IntersectionObserver instead of viewportCallback + scroll detection. Oct 13, 2020
@samouri samouri marked this pull request as ready for review October 13, 2020 21:27
@amp-owners-bot
Copy link

Hey @alanorozco! These files were changed:

extensions/amp-3q-player/0.1/amp-3q-player.js
extensions/amp-nexxtv-player/0.1/amp-nexxtv-player.js
extensions/amp-ooyala-player/0.1/amp-ooyala-player.js
extensions/amp-powr-player/0.1/amp-powr-player.js
extensions/amp-wistia-player/0.1/amp-wistia-player.js

@samouri samouri requested review from dvoytenko and removed request for micajuine-ho October 13, 2020 21:28
@samouri samouri self-assigned this Oct 13, 2020
@samouri samouri mentioned this pull request Oct 14, 2020
Closed
dvoytenko
dvoytenko suggested changes Oct 15, 2020
View reviewed changes
src/service/video-manager-impl.js Outdated Show resolved Hide resolved
src/service/video-manager-impl.js Outdated Show resolved Hide resolved
src/service/video-manager-impl.js Outdated Show resolved Hide resolved
@samouri samouri force-pushed the remove-video-vpcb branch 2 times, most recently from 735f499 to 1f73979 Compare October 19, 2020 17:14
@samouri samouri mentioned this pull request Oct 19, 2020
Closed
@samouri
Copy link
Member Author

samouri commented Oct 20, 2020

I've extracted getViewportObserver() into #30761. Will rebase once that goes through

@samouri
Copy link
Member Author

samouri commented Oct 21, 2020 •
edited

@alanorozco: can you please review this but ignore src/viewport-observer? It is being handled in a separate PR

@samouri samouri force-pushed the remove-video-vpcb branch 2 times, most recently from 05b545c to 1ff7016 Compare October 23, 2020 15:46
This was referenced Oct 23, 2020
Closed
Closed
@samouri samouri added Component: amp-video & interface amp-video and video interface/manager WG: components labels Oct 23, 2020
@amp-bundle-size amp-bundle-size bot requested a review from rsimha October 24, 2020 03:48
This was referenced Oct 26, 2020
Merged
Closed
@samouri
Copy link
Member Author

samouri commented Oct 27, 2020

@alanorozco: I plan on merging later today. Any feedback you leave before/after I'd be happy to followup with!

@samouri samouri merged commit 467a57a into ampproject:master Oct 27, 2020
@samouri samouri deleted the remove-video-vpcb branch October 27, 2020 19:03
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
@samouri @ed-bird
AMP Video Players: use IntersectionObserver instead of viewportCallba…
7cc63b2 
…ck + scroll detection. (ampproject#30647)

* amp-video: remove all traces of viewportCallback

* devAssert

* devAssert --> toWin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvoytenko dvoytenko dvoytenko approved these changes

@alanorozco alanorozco Awaiting requested review from alanorozco

@rsimha rsimha Awaiting requested review from rsimha

Assignees

@samouri samouri

Labels
cla: yes Component: amp-video & interface amp-video and video interface/manager PR use: In Beta / Experimental WG: components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AMP Ima Video pause upon scrolling page
3 participants
@samouri @dvoytenko @ampprojectbot