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

Add baseline infrastructure for animations in amp-story #11525

Merged
merged 4 commits into from
Oct 2, 2017

Conversation

alanorozco
Copy link
Member

General design aspects

  • Keyframe format is a subset of the amp-animation format (i.e. more restrictions)
  • Presets are used in elements that contain the animate-in attribute. Other animation properties are configured with the attributes animate-in-duration, animate-in-delay and animate-in-after.
  • animation-presets.js defines the preset animations that can be used in stories.
  • AnimationRunner handles the entrance animation defined for a given element.
  • AnimationManager handles the entrance animations for a given story page.
  • AnimationSequence is a bus shared by all AnimationRunners for sequencing.

Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Thanks for the design overview. Feel free to address minor comments in a follow-up.

*/

/** @typedef {!Array<!Object<string, *>>} */
export let Keyframes;

Choose a reason for hiding this comment

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

Suffix typedefs with "Def", i.e. KeyFramesDef.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

constructor(
page, animationDef, webAnimationBuilderPromise, vsync, timer, sequence) {
/** @private @const */
this.page_ = page;

Choose a reason for hiding this comment

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

Types for these ivars?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are they needed here? Since they're @const and directly assigned from a typed param, Closure could then do inference, right?

import {setStyle, resetStyles} from '../../../src/style';


const ANIMATE_IN_ATTRIBUTE_NAME = 'animate-in';

Choose a reason for hiding this comment

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

/** @const {string} */

IIRC, Closure is pretty dumb about this so we have to do some handholding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


/**
* @param {!PlaybackActivity} activity
* @param {!Promise=} opt_wait

Choose a reason for hiding this comment

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

@private

IIRC, Closure needs this annotation for obfuscation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oversight on my part, thanks.

/**
* Executes playback activity if runner is ready.
* @param {!PlaybackActivity} activity
* @param {?Promise} wait

Choose a reason for hiding this comment

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

@private

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

*/
notifyFinish(id) {
if (id in this.subscriptionPromises_) {
dev().assert(this.subscriptionResolvers_)[id]();

Choose a reason for hiding this comment

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

Did you mean dev().assert(this.subscriptionResolvers_[id])()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup!

}

/**
* @param {string} name

Choose a reason for hiding this comment

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

@param {!Element} el?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops

*/
createAnimationBuilderPromise_() {
return Services.extensionsFor(this.ampdoc_.win)
.loadExtension('amp-animation')

Choose a reason for hiding this comment

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

You probably want installExtensionForDoc here which will de-dupe loads. loadExtension was renamed preloadExtension during a refactor for better AmpDoc support.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks.


// TODO(alanorozco): Looping animations
/** Manager for animations in story pages. */
export class AnimationManager {

Choose a reason for hiding this comment

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

Minor: This file is large enough to split into two IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will follow-up



/** Bus for animation sequencing. */
class AnimationSequence {

Choose a reason for hiding this comment

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

signals.js does something pretty similar FYI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's cool. I'll look into whether the bus can be replaced with this.

@ampprojectbot ampprojectbot added this to the Pending Triage milestone Oct 2, 2017
@alanorozco alanorozco merged commit 757fb67 into ampproject:master Oct 2, 2017
@alanorozco alanorozco moved this from Awaiting Review to Merged in Stories - By Type Oct 2, 2017
@alanorozco alanorozco deleted the story-animation branch October 2, 2017 22:17
@newmuis newmuis moved this from Migration to amphtml repo to Done in Stories - By Type Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants