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

Pause/Resume and Layout/Unlayout callbacks #1637

Merged
merged 11 commits into from
Mar 18, 2016
Merged

Pause/Resume and Layout/Unlayout callbacks #1637

merged 11 commits into from
Mar 18, 2016

Conversation

jridgewell
Copy link
Contributor

First pass at separating pauseCallback from unlayoutCallback, which adds resumeCallback to the mix.

  • Setting up the state machine needs work
  • Need integration tests on to check that doc/viewer visibility changes are detected and issue pause/resume and unlayout.

Fixes #2560.

Future PRs:

  • Move layoutCallback calls into the state machine transitions
  • Get rid of the boolean API from unlayoutCallback

@jridgewell jridgewell force-pushed the conserveResourcesCallback branch 2 times, most recently from 793eeb5 to 388fd0e Compare February 4, 2016 20:12
@jridgewell jridgewell force-pushed the conserveResourcesCallback branch 2 times, most recently from 73be9a0 to e9a23b1 Compare February 5, 2016 21:50
@jridgewell jridgewell force-pushed the conserveResourcesCallback branch 2 times, most recently from 1c8b8b9 to 7399177 Compare February 9, 2016 22:52
@jridgewell jridgewell assigned dvoytenko and unassigned cramforce Feb 10, 2016
@jridgewell
Copy link
Contributor Author

It passed Travis!!! 🎆🎊

dvoytenko
dvoytenko reviewed Feb 10, 2016
View reviewed changes
src/base-element.js
* || documentInactiveCallback may be called N times after this.
* || pauseCallback may be called N times after this.
* || resumeCallback may be called N times after this.
* || unlayoutCallback may be called N times after this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlayout before layout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed to only be called if LAYOUT_COMPLETE.

@dvoytenko
Copy link
Contributor

@jridgewell to confirm, as discussed offline, let's do the PR where we add Viewer.getVisibilityState() with an appropriate callback and we'll circle back to this PR after?

@jridgewell
Copy link
Contributor Author

let's do the PR where we add Viewer.getVisibilityState() with an appropriate callback and we'll circle back to this PR after?

Yup.

@jridgewell jridgewell force-pushed the conserveResourcesCallback branch 2 times, most recently from 59e82d6 to f102c53 Compare February 24, 2016 20:18
@jridgewell jridgewell mentioned this pull request Mar 1, 2016
Closed
@jridgewell
Copy link
Contributor Author

Once Travis passes, I'll merge.

@jridgewell jridgewell force-pushed the conserveResourcesCallback branch 2 times, most recently from e3b89ca to c975e8b Compare March 18, 2016 17:30
jridgewell added a commit that referenced this pull request Mar 18, 2016
@jridgewell
Merge pull request #1637 from jridgewell/conserveResourcesCallback
3ebc410 
Pause/Resume and Layout/Unlayout callbacks
@jridgewell jridgewell merged commit 3ebc410 into ampproject:master Mar 18, 2016
@jridgewell jridgewell deleted the conserveResourcesCallback branch March 18, 2016 18:27
@cramforce cramforce mentioned this pull request Mar 18, 2016
Closed
@jridgewell jridgewell mentioned this pull request Feb 3, 2017
Closed
@ampprojectbot ampprojectbot mentioned this pull request Jun 29, 2020
Closed
@ampprojectbot ampprojectbot mentioned this pull request Jan 28, 2021
Open
@ampprojectbot ampprojectbot mentioned this pull request Mar 4, 2021
Open
@elijahsoria elijahsoria mentioned this pull request May 5, 2021
Merged
@ampprojectbot ampprojectbot mentioned this pull request Jul 26, 2021
Open
This was referenced Jan 19, 2022
Open
Open
This was referenced Mar 17, 2023
Open
Open
Open
This was referenced Sep 5, 2023
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dvoytenko dvoytenko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jridgewell @dvoytenko @mkhatib @cramforce