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

Animation compositions #9614

Merged
merged 4 commits into from May 31, 2017
Merged

Animation compositions #9614

merged 4 commits into from May 31, 2017

Conversation

dvoytenko
Copy link
Contributor

A relatively small refactoring to allow animations to "include" other animations. The format extensions are described in the modified amp-animation.md.

Partial for #9129.

this.css_ = new CssContextImpl(win, rootNode, baseUrl);

/** @const @private */
this.vsync_ = vsync;
Copy link
Member

Choose a reason for hiding this comment

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

Why passing the vsync instance as opposed to vsyncFor(win)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easier to test. This is an internal class - the main reason is to split the code and test.

*/
constructor(win, rootNode, baseUrl, validate) {
constructor(builder, css, path, target, vars, timing) {
Copy link
Member

Choose a reason for hiding this comment

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

opt_timing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not optional. Only nullable.

return new WebAnimationRunner(this.requests_);
});
/** @const @private {!Array<!Promise>} */
this.deps_ = [];
Copy link
Member

Choose a reason for hiding this comment

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

deps for what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dependencies to fully resolve all animation requests. Some can only be done asynchronously. I added the description.

this.resolveTargets_(spec) :
[null];
targets.forEach(target => {
this.target_ = target || prevTarget;
Copy link
Member

Choose a reason for hiding this comment

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

why does this fallback to prevTarget?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

prevTarget is basically the "parent" target. Either a nested animation overrides it or the "parent" target is used. The mergeVars_ and mergeTiming_ that follow, essentially do the same thing.

<amp-animation id="anim1" layout="nodisplay">
<script type="application/json">
{
"animamtion": "anim2",
Copy link
Member

Choose a reason for hiding this comment

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

typo: animation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

<script type="application/json">
{
"selector": ".target-class",
"animamtion": "anim2",
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -299,6 +333,64 @@ can be reduced to an array of components. For instance:
</amp-animation>
```


### Animation composition
Copy link
Member

Choose a reason for hiding this comment

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

Can this clarify further on the difference between setting animation as a reference vs. animations? Some confusion can come from the fact that they can be described top-level or nested one level down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added possible reasons. Similar to function call composition, two reasons: allow reuse across multiple animations (as with calling function from different places) or simply making each animation more manageable (as with splitting unwieldy function into two, even if there's no reuse).

@dvoytenko
Copy link
Contributor Author

@alanorozco All comments addressed. PTAL.

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.

None yet

4 participants