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.

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

V1: allow lightbox to add its own intersection observer to the scheduler #33281

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

dvoytenko
Copy link
Contributor

@dvoytenko dvoytenko commented Mar 15, 2021

Partial for #31915.
Partial for #31540.

Key changes:

  • A container element, such as amp-lightbox can contribute its own intersection observer to the scheduler.
  • unmountAll is used from the new resources-container-helper.js.

TODO:

@dvoytenko dvoytenko requested review from jridgewell and samouri March 15, 2021 22:08
@dvoytenko dvoytenko force-pushed the run3/mount2-container branch 2 times, most recently from 8a9809c to a88b2e1 Compare March 18, 2021 17:16
@dvoytenko dvoytenko marked this pull request as ready for review March 18, 2021 17:17
Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a confused why this PR only adds to amp-lightbox as opposed to also being able to remove things. Is there no pre-existing functionality this replaces?

I.e.: does unmountAllWithin also automatically pause all of its subresources? If so we should be able to remove the pause.

src/utils/resource-container-helper.js Outdated Show resolved Hide resolved
src/service/scheduler.js Show resolved Hide resolved
@dvoytenko
Copy link
Contributor Author

I'm a confused why this PR only adds to amp-lightbox as opposed to also being able to remove things. Is there no pre-existing functionality this replaces?

It both adds and removes the container for amp-lightbox. Few other elements would quickly follow.

I.e.: does unmountAllWithin also automatically pause all of its subresources? If so we should be able to remove the pause.

Correct. Unmount also pauses. However, for now, "pause" is still useful in its own right. A little less useful with async pause, but it's still used and maybe would be used in perpetuity.

src/custom-element.js Outdated Show resolved Hide resolved
src/base-element.js Outdated Show resolved Hide resolved
src/dom.js Outdated Show resolved Hide resolved
src/utils/resource-container-helper.js Outdated Show resolved Hide resolved
src/service/scheduler.js Show resolved Hide resolved
src/service/scheduler.js Show resolved Hide resolved
src/service/scheduler.js Outdated Show resolved Hide resolved
@dvoytenko dvoytenko requested review from jridgewell and samouri March 24, 2021 00:08
@dvoytenko dvoytenko force-pushed the run3/mount2-container branch from 14d60c8 to 12a4961 Compare March 24, 2021 22:31
src/base-element.js Outdated Show resolved Hide resolved
src/custom-element.js Outdated Show resolved Hide resolved
@dvoytenko dvoytenko force-pushed the run3/mount2-container branch from d140d66 to ee19595 Compare March 25, 2021 04:21
@dvoytenko dvoytenko merged commit afa1a15 into ampproject:master Mar 25, 2021
@dvoytenko dvoytenko deleted the run3/mount2-container branch March 25, 2021 04:53
@samouri
Copy link
Member

samouri commented Mar 25, 2021

When is opt_scroller used or planned to be used?

@dvoytenko
Copy link
Contributor Author

When is opt_scroller used or planned to be used?

I think all Bento components separate the scroller from the root element to avoid modifying user markup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants