Add amp-fx="fade-in" - scroll triggered timed animation. #14351

nainar · 2018-04-02T16:20:36Z

This PR fixes #14351 adds the following functionality:

adds the ability to accept amp-fx="fade-in" which triggers a timed fade in animation
customizing the fade in animation by specifying:
- duration of animation
- easing curve of the animation
- margin at which the animation should be triggered

…ccordingly as well

…rallax_rewrite

nainar

I have highlighted the sections that need review. Since #14276 hasn't landed yet the diff has changes made in this PR and that one. Will rebase once #14276 has landed.

nainar · 2018-04-02T16:24:58Z

examples/article-fade-in.amp.html

@@ -0,0 +1,112 @@
+<!doctype html>


New compared to #14276

nainar · 2018-04-02T16:25:13Z

extensions/amp-fx-collection/0.1/amp-fx-collection.js

@@ -134,6 +128,10 @@ class AmpFxCollection {

    // Validate that we support the requested fx types.
    fxTypes.forEach(fxType => {
+      if (fxType == FxType.FADE_IN) {


New compared to #14276

Can this be moved to the preset?

nainar · 2018-04-02T16:25:22Z

extensions/amp-fx-collection/0.1/providers/amp-fx-presets-utils.js

@@ -0,0 +1,47 @@
+/**


New compared to #14276

nainar · 2018-04-02T16:25:29Z

extensions/amp-fx-collection/0.1/providers/amp-fx-presets.js

+      }
+    },
+  },
+  'fade-in': {


New compared to #14276

nainar · 2018-04-02T16:25:37Z

extensions/amp-fx-collection/0.1/providers/fx-provider.js

 import {Services} from '../../../../src/services';
-import {dev, user} from '../../../../src/log';
+import {convertEasingKeyword} from './amp-fx-presets-utils';


New compared to #14276

nainar · 2018-04-02T16:26:18Z

extensions/amp-fx-collection/0.1/test/validator-amp-fx-collection.html

@@ -33,5 +33,10 @@
  <div>
    <h1 amp-fx="parallax"></h1>
  </div>
+  <div amp-fx="fade-in"></div>


New compared to #14276

nainar · 2018-04-02T16:26:24Z

extensions/amp-fx-collection/0.1/test/validator-amp-fx-collection.out

@@ -34,5 +34,10 @@ PASS
 |    <div>
 |      <h1 amp-fx="parallax"></h1>
 |    </div>
+|    <div amp-fx="fade-in"></div>


New compared to #14276

nainar · 2018-04-02T16:26:31Z

extensions/amp-fx-collection/amp-fx-collection.md

@@ -79,6 +79,38 @@ In this example, as the user scrolls the page, the h1 element scrolls faster rel
 </h1>
 ```

+### fade-in


New compared to #14276

Awesome, thanks for updating the docs in the same PR!

I would add a note about fade-in being experimental.

nainar · 2018-04-02T16:26:36Z

tools/experiments/experiments.js

@@ -288,6 +288,12 @@ const EXPERIMENTS = [
    spec: 'https://github.com/ampproject/amphtml/issues/9074',
    cleanupissue: 'https://github.com/ampproject/amphtml/issues/14263',
  },
+  {


New compared to #14276

nainar · 2018-04-02T16:26:41Z

validator/validator-main.protoascii

@@ -4524,7 +4524,7 @@ attr_lists: {
  # https://www.ampproject.org/docs/reference/components/amp-fx-collection
  attrs: {
    name: "amp-fx"
-    value_casei: "parallax"
+    value_regex_casei: "(fade-in|parallax)"


New compared to #14276

nainar · 2018-04-02T23:18:33Z

@alanorozco I have rebased on to the rewrite in #14276 so this should be much easier to read now.

alanorozco · 2018-04-03T15:50:28Z

extensions/amp-fx-collection/0.1/providers/amp-fx-presets.js

+      }
+
+      // If above the threshold of trigger-position
+      if (!fxElement.isMutateScheduled()) {


Short-circuit here.

Maybe changing getResources() to mutateElement directly can save you a couple of steps (setIsMutateScheduled(), getElement()).

Arrow func.

if (fxElement.isMutateScheduled()) { return; } fxElement.mutate(() => { // ... });

I agree with the short circuiting here. However FxElement doesn't extend BaseElement so I don't think the mutateElement will work here, will it?

alanorozco · 2018-04-03T16:27:57Z

extensions/amp-fx-collection/0.1/providers/fx-provider.js

@@ -110,6 +118,19 @@ export class FxElement {

    /** @private {number} */
    this.factor_ = parseFloat(element.getAttribute('data-parallax-factor'));
+


What worries me about this style is that the specific FX are being scattered in more than one place.

For generic properties, like easing or duration, data-duration (without the preset name) should work fine. If they are to be invalid for certain presets, we can add validation rules.

I share your concern here. Made the change. We already have one preset specific attribute data-parallax-factor but I am going to talk to Ali about renaming that. That is a discussion for another thread though.

alanorozco · 2018-04-03T16:28:39Z

extensions/amp-fx-collection/amp-fx-collection.md

@@ -79,6 +79,38 @@ In this example, as the user scrolls the page, the h1 element scrolls faster rel
 </h1>
 ```

+### fade-in


Awesome, thanks for updating the docs in the same PR!

I would add a note about fade-in being experimental.

alanorozco · 2018-04-03T16:29:20Z

extensions/amp-fx-collection/0.1/amp-fx-collection.js

@@ -134,6 +128,10 @@ class AmpFxCollection {

    // Validate that we support the requested fx types.
    fxTypes.forEach(fxType => {
+      if (fxType == FxType.FADE_IN) {


Can this be moved to the preset?

nainar

Uploaded a new commit with the changes you asked for. PTAL?

nainar · 2018-04-03T16:35:39Z

extensions/amp-fx-collection/amp-fx-collection.md

@@ -79,6 +79,38 @@ In this example, as the user scrolls the page, the h1 element scrolls faster rel
 </h1>
 ```

+### fade-in


nainar · 2018-04-03T16:36:53Z

extensions/amp-fx-collection/0.1/providers/fx-provider.js

@@ -110,6 +118,19 @@ export class FxElement {

    /** @private {number} */
    this.factor_ = parseFloat(element.getAttribute('data-parallax-factor'));
+


I share your concern here. Made the change. We already have one preset specific attribute data-parallax-factor but I am going to talk to Ali about renaming that. That is a discussion for another thread though.

nainar · 2018-04-03T18:17:28Z

extensions/amp-fx-collection/0.1/amp-fx-collection.js

@@ -134,6 +128,10 @@ class AmpFxCollection {

    // Validate that we support the requested fx types.
    fxTypes.forEach(fxType => {
+      if (fxType == FxType.FADE_IN) {


nainar · 2018-04-03T18:26:18Z

extensions/amp-fx-collection/0.1/providers/amp-fx-presets.js

+      }
+
+      // If above the threshold of trigger-position
+      if (!fxElement.isMutateScheduled()) {


I agree with the short circuiting here. However FxElement doesn't extend BaseElement so I don't think the mutateElement will work here, will it?

…ade_in

* Revision bump for #14351 * Revision bump and whitespace correction for #14464

* WIP * Refactor parallax code to make it easier to add new presets * Force Travis checks * Travis fixes * Private -> protected * Add support for amp-fx='fade-in'. Write example and validator tests accordingly as well * Demos post UX review * Add 'real world' example that uses amp-fx='fade-in' * Commit worthy test * Add support for easing keywords * Only access private members through accessors * Only access private members through accessors * Remove demo files * Add accessor for resources. * Fix linter errors * Formatting FUNS * Fix markdown * Moar demos * Gate amp-fx='fade-in' experiment * Edits before push * markdown changes * Bring back demos * remove US demos * Respond to alanorozco@'s comments * Fix build failures * Fix missing support check * Travis fixes * Cleanups * Remove references to fade-in * Don't use private members outside class * OOPS

* Revision bump for #14351 * Revision bump and whitespace correction for #14464

nainar added 24 commits March 27, 2018 17:59

WIP

fa4cdf3

Refactor parallax code to make it easier to add new presets

69744ef

Force Travis checks

fa3be33

Travis fixes

c392ebd

Private -> protected

ed9501e

Add support for amp-fx='fade-in'. Write example and validator tests a…

9cf006b

…ccordingly as well

Demos post UX review

5e5b594

Add 'real world' example that uses amp-fx='fade-in'

a7c956a

Commit worthy test

62e019b

Add support for easing keywords

ab2d31c

Only access private members through accessors

70190e5

Merge branch 'amp_fx_parallax_rewrite' into amp_new_fade_in

db7790e

Only access private members through accessors

3690139

Merge branch 'master' of github.com:ampproject/amphtml into amp_fx_pa…

5fb43b6

…rallax_rewrite

Merge branch 'amp_fx_parallax_rewrite' into amp_new_fade_in

2ed4015

Remove demo files

bf0a305

Add accessor for resources.

6a9a2aa

Fix linter errors

0a0d8b5

Formatting FUNS

b1edb60

Fix markdown

c31a53c

Moar demos

2ef11f8

Merge branch 'amp_fx_parallax_rewrite' into amp_new_fade_in

90e17ac

Gate amp-fx='fade-in' experiment

5e20174

Edits before push

5a72905

googlebot added the cla: yes label Apr 2, 2018

nainar commented Apr 2, 2018

View reviewed changes

nainar requested a review from alanorozco April 2, 2018 16:28

nainar added 3 commits April 2, 2018 10:12

markdown changes

a31fa38

Bring back demos

b2e5c21

Retrigger Travis checks

1d30eac

nainar added 3 commits April 2, 2018 16:05

Merge branch 'amp_fx_parallax_rewrite' into amp_new_fade_in

55ee13b

Pull in parallax rewrite

367431c

remove US demos

6bd35ef

alanorozco requested changes Apr 3, 2018

View reviewed changes

Respond to alanorozco@'s comments

cedc96a

nainar commented Apr 3, 2018

View reviewed changes

nainar added 8 commits April 3, 2018 13:48

Fix build failures

0a26c2a

Fix missing support check

99bbfb2

Travis fixes

c134a14

Cleanups

eb45ecb

Merge branch 'master' of github.com:ampproject/amphtml into amp_new_f…

91bf639

…ade_in

Remove references to fade-in

64e4dc2

Don't use private members outside class

c503318

OOPS

8d35309

alanorozco approved these changes Apr 5, 2018

View reviewed changes

cvializ merged commit e554769 into ampproject:master Apr 5, 2018

powdercloud added a commit to powdercloud/amphtml that referenced this pull request Apr 10, 2018

Revision bump for ampproject#14351

9e826e2

powdercloud added a commit that referenced this pull request Apr 10, 2018

Upstream more validator changes (#14509)

a4823f6

* Revision bump for #14351 * Revision bump and whitespace correction for #14464

aghassemi mentioned this pull request Apr 16, 2018

Status Update 2018-04-02 #14329

Closed

nainar mentioned this pull request Apr 17, 2018

Implement <amp-fx-collection> fade-in #14150

Closed

aghassemi mentioned this pull request Apr 18, 2018

Status Update 2018-04-16 #14616

Closed

glevitzky pushed a commit that referenced this pull request Apr 27, 2018

Upstream more validator changes (#14509)

e9a3dd6

* Revision bump for #14351 * Revision bump and whitespace correction for #14464

nainar mentioned this pull request Apr 27, 2018

[Master feature] Easy fade-in-on-scroll #14067

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add amp-fx="fade-in" - scroll triggered timed animation. #14351

Add amp-fx="fade-in" - scroll triggered timed animation. #14351

nainar commented Apr 2, 2018 •

edited

Loading

nainar left a comment

nainar Apr 2, 2018

nainar Apr 2, 2018

alanorozco Apr 3, 2018

nainar Apr 3, 2018

nainar Apr 2, 2018

nainar Apr 2, 2018

nainar Apr 2, 2018

nainar Apr 2, 2018

nainar Apr 2, 2018

nainar Apr 2, 2018

alanorozco Apr 3, 2018

nainar Apr 3, 2018

nainar Apr 2, 2018

nainar Apr 2, 2018

nainar commented Apr 2, 2018

alanorozco Apr 3, 2018

nainar Apr 3, 2018

alanorozco Apr 3, 2018

nainar Apr 3, 2018

alanorozco Apr 3, 2018

alanorozco Apr 3, 2018

nainar left a comment

nainar Apr 3, 2018

nainar Apr 3, 2018

nainar Apr 3, 2018

nainar Apr 3, 2018

		@@ -110,6 +118,19 @@ export class FxElement {

		/** @private {number} */
		this.factor_ = parseFloat(element.getAttribute('data-parallax-factor'));

Add amp-fx="fade-in" - scroll triggered timed animation. #14351

Add amp-fx="fade-in" - scroll triggered timed animation. #14351

Conversation

nainar commented Apr 2, 2018 • edited Loading

nainar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nainar commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nainar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nainar commented Apr 2, 2018 •

edited

Loading