Animations: width, height and rand functions#9539
Animations: width, height and rand functions#9539dvoytenko merged 5 commits intoampproject:masterfrom
Conversation
alanorozco
left a comment
There was a problem hiding this comment.
Approach looks great. Just a few readability concerns.
| } | ||
|
|
||
|
|
||
| /** |
|
|
||
| /** @override */ | ||
| css() { | ||
| // No CSS represention. |
There was a problem hiding this comment.
Can we log this? This can lead to subtle bugs if this gets called expecting a valid result state.
There was a problem hiding this comment.
Now asserting it via exception.
| } | ||
|
|
||
| /** @override */ | ||
| css() { |
| */ | ||
| export class CssRandNode extends CssNode { | ||
| /** | ||
| * @param {?CssNode=} left |
There was a problem hiding this comment.
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.
| let element; | ||
| try { | ||
| if (selectionMethod == 'closest') { | ||
| element = closestBySelector(this.requireTarget_(), selector); |
There was a problem hiding this comment.
Can we have an enum-like constant for this? Mainly to document the different methods and how they behave.
There was a problem hiding this comment.
I'll add if we have more than one - an enum of one is too little :)
aghassemi
left a comment
There was a problem hiding this comment.
lgtm, one concern regarding reevaluating width/height when DOM mutations happen.
| return false; | ||
| } | ||
|
|
||
| /** @override */ |
There was a problem hiding this comment.
Do we handle size changes? looks like now we need to make sure amp-animation reevaluates the expressions onChange
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
Is there code that handles this?
There was a problem hiding this comment.
Yes - in the ast class. And there are rules in this file on parsing.
Partial for #9129.
/cc @lexandera