Skip to content

Force layout of elements used in animations#9150

Merged
dvoytenko merged 5 commits intoampproject:masterfrom
dvoytenko:anim9
May 5, 2017
Merged

Force layout of elements used in animations#9150
dvoytenko merged 5 commits intoampproject:masterfrom
dvoytenko:anim9

Conversation

@dvoytenko
Copy link
Copy Markdown
Contributor

Closes #7921.

@dvoytenko dvoytenko requested a review from aghassemi May 4, 2017 20:52
Dima Voytenko added 2 commits May 4, 2017 13:54
Resource.setOwner(element, owner);
}

/**
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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()) {
Copy link
Copy Markdown
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
Copy Markdown
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