🐛Tweaks "ken burns" animations to check for image sizes by Enriqe · Pull Request #14334 · ampproject/amphtml

Enriqe · 2018-03-30T22:21:15Z

"Pan" animations will not look as intended with screen sizes bigger than the image.

This PR addresses this issue by checking if the images are smaller than the screen size and scaling it accordingly.

It also checks if the image is a direct child of a grid layer using a "fill" template, and overwrites the styles so that the calculations of the animation won't be affected.

Enriqe · 2018-03-30T22:34:39Z

/cc @newmuis

newmuis · 2018-03-30T22:40:49Z

extensions/amp-story/0.1/animation-presets-utils.js

+                      dimensions.pageWidth / dimensions.targetWidth : 1;
+  const heightFactor = dimensions.pageHeight > dimensions.targetHeight ?
+                       dimensions.pageHeight / dimensions.targetHeight : 1;
+  return Math.max(widthFactor, heightFactor) + 0.25;


Maybe instead of adding 0.25, we should multiply by 1.25? If you need to increase something by 6000x for it to fit the viewport, increasing it by 6000.25 won't give you much space to pan.

Also, nit: whatever approach we take, that magic number should be a constant

Enriqe · 2018-03-30T23:04:14Z

extensions/amp-animation/0.1/web-animation-types.js

 const WHITELISTED_RPOPS = {
  'opacity': true,
  'transform': true,
+  'transform-origin': true,


hey @aghassemi, question: if I would like to whitelist this property, do I have to do something on the validator side as well?

No need for validator changes. It is all runtime validation

newmuis · 2018-04-02T16:21:47Z

extensions/amp-story/0.1/animation-presets-utils.js

@@ -83,8 +83,8 @@ export function whooshIn(startX, startY, endX, endY) {
 * @param {StoryAnimationDimsDef} dimensions Dimensions of page and target.
 */
 export function pageIsLargerThanTarget(dimensions) {


With the change to the implementation, should this be renamed? pageIsLargerThanOrEqualToTarget(...)?
targetFitsWithinPage(...)?

newmuis · 2018-04-02T16:22:10Z

extensions/amp-story/0.1/animation-presets-utils.js

@@ -83,8 +83,8 @@ export function whooshIn(startX, startY, endX, endY) {
 * @param {StoryAnimationDimsDef} dimensions Dimensions of page and target.


nit: missing @return

…smaller than screen and scale them accordingly.

…de a grid layer fill template.

Enriqe · 2018-04-06T16:31:35Z

As discussed offline, I will look into adding the logic of overwriting the styles for the image on the animation itself, rather than in the grid-layer logic.

Enriqe · 2018-04-09T20:19:29Z

Demos:

https://quiet-caverns-98958.herokuapp.com/examples/amp-story/animations-presets.html

https://quiet-caverns-98958.herokuapp.com/examples/amp-story/visual-effects.html

newmuis · 2018-04-09T21:26:58Z

extensions/amp-story/0.1/animation-presets.js

+  const fillTemplate = 'fill';
+
+  // For panning and zooming animations.
+  if (PANNING_ANIMATION_NAMES.includes(presetName)) {


I'm not sure we can use includes; can you use PANNING_ANIMATION_NAMES.indexOf(presetName) >= 0?

Sure. Out of curiosity, why wouldn't we want to use includes?

It might be okay? I'm not 100% sure. I thought I remembered that some things that are added with es6 are not transpiled, but I never know which ones.

newmuis · 2018-04-09T21:27:24Z

extensions/amp-story/0.1/animation-presets.js

+ * @private @const {!Object<string, string>}
+ */
+const ANIMATION_CSS_CLASS_NAMES = {
+  'pan': 'i-amphtml-story-grid-template-with-pan-animation',


Does this problem only occur with pan? i.e. Do zoom animations ever show whitespace?

I don't think we want to apply this styling to zoom for a couple of reasons:

It's fair to assume that all the use cases for panning will have to do with images that outgrow the size of the viewport (from the name of the animation), but I'm not so sure for zooming. e.g. Users may want to zoom in on an object that just takes a fraction of the screen.

If the fill layout resizes the image to be the same size of the viewport, that wouldn't break the animation, since zoom doesn't depend on the image dimensions. It's just a simple scale.

WDYT?

Thinking about it more, we might want to also include zoom animations. It will make it more straight forward for when users want to combine both the pan and the zoom. This way they won't have to wrap everything inside a div. I'll do a quick prototype and see if it works.

SGTM; I think especially if it's a child of a template="fill" layer, we might want this.

Prototyped it with the zooming and it works well. I think we should also include it since it eliminates the need to wrap images inside a div when using multiple images for the animation.

newmuis · 2018-04-10T17:03:33Z

extensions/amp-story/0.1/animation-presets.js

 * @private @const {!Array<string>}
 */
-const PANNING_ANIMATION_NAMES = [
+const FULL_SCREEN_ANIMATION_NAMES = [


nit: slightly prefer "full bleed" over "full screen" here, since technically it's not fullscreen as long as we have the browser bars (and, at some point, we might have an actual "full screen" implementation that is different from this)

Enriqe · 2018-04-11T19:06:35Z

/cc @gmajoulet @alanorozco

newmuis · 2018-04-11T19:59:17Z

extensions/amp-story/0.1/amp-story.css

+.i-amphtml-story-grid-template-with-full-bleed-animation {
+  position: absolute !important;
+  display: block !important;
+  padding: 0;


This should either be !important or in amp-story-user-overridable.css.

newmuis · 2018-04-11T20:04:16Z

extensions/amp-story/0.1/animation-presets.js

    duration: 1000,
    easing: 'linear',
    keyframes(dimensions) {
+      let resized = false;


Maybe this whole keyframes def can be extracted into a function that just accepts frames as a parameter? As is, this repeats a lot of the same code.

Not sure with what you meant. I made some changes to how these presets are calculated though, does your suggestion still stand after those?

newmuis · 2018-04-11T20:04:53Z

extensions/amp-story/0.1/test/test-full-bleed-animations.js

+    const offsetX = -dimensions.targetWidth / 2;
+    const offsetY = dimensions.pageHeight - dimensions.targetHeight;
+
+    const expectedKeyframes = [{


style nit: Maybe put the { on the next line and indent both objects +2

newmuis · 2018-04-11T20:05:46Z

extensions/amp-story/0.1/test/test-full-bleed-animations.js

+  }
+
+  it('should tell if target fits within page', () => {
+    let dimensions = setDimensions(380 , 580, 360, 580);


If these are three different cases, we should break them into three different it(...) declarations. Especially true for the one that's expected to be false

newmuis · 2018-04-11T20:10:28Z

extensions/amp-story/0.1/test/test-full-bleed-animations.js

+        });
+  });
+
+  it('should not add additional css class to non-full-bleed animation target ' +


optional: this name and the two above are a bit hard to parse. I tried to come up with something clearer, but I now see that these are just super specific cases with a lot to say 😄

Rephrased it a bit in hopes of making it easier to read 😅

newmuis · 2018-04-11T20:13:20Z

extensions/amp-story/0.1/animation-presets.js

+  const fillTemplate = 'fill';

+  // For full bleed animations.
+  if (FULL_BLEED_ANIMATION_NAMES.indexOf(presetName) >= 0) {


Actually, this can just be === or a switch statement instead of indexOf, right? Sorry for the churn, commented too quickly last time.

Not 100% sure what you mean. Do you want to check for every preset and see if it equals the current preset, then apply the corresponding styles?

newmuis · 2018-04-11T20:14:57Z

extensions/amp-story/0.1/animation-presets.js

+ * @param {string} presetName
+ */
+export function setStyleForPreset(el, presetName) {
+  const fullBleed = 'full-bleed';


Extract this to a top-level constant and change the name to SNAKE_CASE. Then, you can also use the constant as the key for ANIMATION_CSS_CLASS_NAMES too:

const ANIMATION_CSS_CLASS_NAMES = { [FULL_BLEED_CATEGORY]: 'i-amphtml-story-grid-template-with-full-bleed-animation', };

newmuis · 2018-04-11T20:18:22Z

extensions/amp-story/0.1/animation-presets-utils.js

+ * @param {number} scalingFactor Scaling factor at which target will be scaled.
+ * @return {KeyframesDef}
+ */
+export function enlargeKeyFrames(keyframes, scalingFactor) {


Maybe just make this scaleAndTranslate(startX, startY, endX, endY, scalingFactor) to follow the pattern. Then, if scalingFactor === 1, you don't have to emit the scale(scalingFactor), so that in animation-presets.js, you can just use scaleAndTranslate instead of translate2d with a conditional.

newmuis · 2018-04-11T20:19:07Z

extensions/amp-story/0.1/animation-presets-utils.js

 }
+
+/**
+ * Checks if the target's dimensions are smaller than or equal to those of the page.


nit: This is ||, so it should be "Checks if either of the target's dimensions are smaller than or equal to those of the page."

newmuis

Looking good! Last few tweaks 😄

newmuis · 2018-04-12T11:35:53Z

extensions/amp-story/0.1/animation-presets-utils.js

+ * @param {number} scalingFactor Scaling factor at which target will be scaled.
+ * @return {KeyframesDef}
+ */
+export function enlargeKeyFrames(keyframes, scalingFactor) {


nit: this no longer needs to be exported.

newmuis · 2018-04-12T11:36:31Z

extensions/amp-story/0.1/animation-presets.js

+ * @param {string} presetName
+ */
+export function setStyleForPreset(el, presetName) {
+  const fillTemplate = 'fill';


nit: this can be top-level too

newmuis · 2018-04-12T11:37:10Z

extensions/amp-story/0.1/test/test-full-bleed-animations.js

@@ -0,0 +1,206 @@
+import {AmpStory} from '../amp-story';


nit: copyright goes before imports

* Tweaks ken burns effect animations to check for image sizes that are smaller than screen and scale them accordingly. * Adds constant for scaling factor, linter stuff. * Linter.: * also take into consideration images that are same size as page. * import * change method name. * Replace CSS class for elements animated with a panning animation inside a grid layer fill template. * Move style specific logic to animation-presets.js and add examples. * Changes includes for indexOf. * Includes zoom animations in the tweak. * Renames from full screen to full bleed. * format html * Comment. * adds test. * Adds more tests. * lint * Reviews. * nit changes * add visible for testing

googlebot added the cla: yes label Mar 30, 2018

Enriqe mentioned this pull request Mar 30, 2018

Adds section for Ken Burns Effect ampproject/amp-by-example#1202

Merged

newmuis reviewed Mar 30, 2018

View reviewed changes

Enriqe commented Mar 30, 2018

View reviewed changes

newmuis reviewed Apr 2, 2018

View reviewed changes

Enriqe added 7 commits April 5, 2018 22:16

Tweaks ken burns effect animations to check for image sizes that are …

be763a6

…smaller than screen and scale them accordingly.

Adds constant for scaling factor, linter stuff.

cb47fce

Linter.:

e4f253a

also take into consideration images that are same size as page.

36be2e7

import

a847552

change method name.

b6303aa

Replace CSS class for elements animated with a panning animation insi…

679a4fd

…de a grid layer fill template.

Move style specific logic to animation-presets.js and add examples.

9042051

Enriqe force-pushed the ken-burns-tweaks branch from 48ddcce to 9042051 Compare April 9, 2018 19:40

newmuis reviewed Apr 9, 2018

View reviewed changes

Enriqe added 2 commits April 9, 2018 18:14

Changes includes for indexOf.

b9b83cf

Includes zoom animations in the tweak.

74b584c

newmuis approved these changes Apr 10, 2018

View reviewed changes

Enriqe added 5 commits April 10, 2018 13:17

Renames from full screen to full bleed.

29075de

format html

4b32fd8

Comment.

8453a06

adds test.

65439bf

Adds more tests.

e69732e

lint

a1834d0

newmuis reviewed Apr 11, 2018

View reviewed changes

Reviews.

c099669

newmuis reviewed Apr 12, 2018

View reviewed changes

Enriqe added 2 commits April 12, 2018 10:22

nit changes

2b37a1f

add visible for testing

1543159

alanorozco approved these changes Apr 12, 2018

View reviewed changes

newmuis merged commit 759f257 into ampproject:master Apr 12, 2018

Enriqe deleted the ken-burns-tweaks branch April 20, 2018 19:29

		@@ -83,8 +83,8 @@ export function whooshIn(startX, startY, endX, endY) {
		* @param {StoryAnimationDimsDef} dimensions Dimensions of page and target.

Conversation

Enriqe commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Enriqe commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enriqe commented Apr 6, 2018

Uh oh!

Enriqe commented Apr 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enriqe commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newmuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Enriqe commented Mar 30, 2018 •

edited

Loading