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

Force layout of elements used in animations #9150

Merged
merged 5 commits into from
May 5, 2017

Conversation

dvoytenko
Copy link
Contributor

Closes #7921.

Dima Voytenko added 2 commits May 4, 2017 13:54
@@ -624,6 +624,39 @@ export class Resources {
}

/**
* Requires the layout of the specified element or top-level sub-elements
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, so in

<amp-selector>
   <amp-layout>
      <amp-img>

resource.requireLayout(ampSelectorComponent); won't layout and wait for <amp-img> because it is not top level? Is there a reason to restrict to top-level sub components (instead of drawing the line at where ownership of sub-components is assumed by parent)?

Copy link
Contributor

Choose a reason for hiding this comment

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

(we chatted offline about this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We currently do not have an option to do recursive filtering well enough. But such a use case is also somewhat rare in animations. I'll follow up on this and see if we can find a way to do this right for all cases.

}
}
if (resource.isDisplayed()) {
promises.push(resource.loadedOnce());
Copy link
Contributor

Choose a reason for hiding this comment

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

should be movable inside the other if( resource.isDisplayed() ) above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Made me realize it was a slight bug - the first isDisplayed/measure should have been inside the promise. Fixed now.

@dvoytenko dvoytenko merged commit 5216703 into ampproject:master May 5, 2017
@dvoytenko dvoytenko deleted the anim9 branch May 5, 2017 16:40
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