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

Animations: width, height and rand functions #9539

Merged
merged 5 commits into from May 26, 2017

Conversation

dvoytenko
Copy link
Contributor

Partial for #9129.

/cc @lexandera

Copy link
Member

@alanorozco alanorozco left a comment

Choose a reason for hiding this comment

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

Approach looks great. Just a few readability concerns.

@@ -596,6 +601,111 @@ export class CssTranslateNode extends CssFuncNode {


/**
* An AMP-specific `width()` and `height()` functions.
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove "An"

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

/** @override */
css() {
// No CSS represention.
return '';
Copy link
Member

Choose a reason for hiding this comment

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

Can we log this? This can lead to subtle bugs if this gets called expecting a valid result state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now asserting it via exception.


/** @override */
css() {
// No CSS represention.
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

export class CssRandNode extends CssNode {
/**
* @param {?CssNode=} left
* @param {?CssNode=} right
Copy link
Member

Choose a reason for hiding this comment

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

It's not fully clear what left and right mean. It looks like it's the threshold for the rand() result but it's not clear how it goes from node -> threshold. Documenting these would be helpful.

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

'Unknown selection method: %s', selectionMethod);
let element;
try {
if (selectionMethod == 'closest') {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have an enum-like constant for this? Mainly to document the different methods and how they behave.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add if we have more than one - an enum of one is too little :)

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

lgtm, one concern regarding reevaluating width/height when DOM mutations happen.

}

/** @override */
calc(context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we handle size changes? looks like now we need to make sure amp-animation reevaluates the expressions onChange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All parts of animations (both CSS3 and Web Animations) explicitly only measure things once before starting and do not change in the course of the animation. That's by design. E.g. if you have a keyframe with transform: translateX(10%) - it's resolved once, e.g. to 10px and doesn't change even if 10% changes. However, <amp-animation> itself listens to resize events and restarts the animation. So it's sort of done.

{R}{A}{N}{D}\( return 'RAND_START'
{W}{I}{D}{T}{H}\( return 'WIDTH_START'
{H}{E}{I}{G}{H}{T}\( return 'HEIGHT_START'
{C}{L}{O}{S}{E}{S}{T}\( return 'CLOSEST_START'
Copy link
Contributor

Choose a reason for hiding this comment

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

Closest?

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there code that handles 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 - in the ast class. And there are rules in this file on parsing.

@dvoytenko dvoytenko merged commit 3b58b93 into ampproject:master May 26, 2017
@dvoytenko dvoytenko deleted the anim14 branch May 26, 2017 17:01
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

6 participants