amp-fx-collection: parallax #13073

aghassemi · 2018-01-26T23:10:53Z

amp-fx-collection extension brings a collection of easy-to-use visual effects to AMP.
parallax is the first one to get implement. Others planned include fade and slide animations.

<h1 amp-fx="parallax" data-parallax-factor="1.4">
<amp-img amp-fx="parallax" data-parallax-factor="0.5">

-This will replace the existing experimental amp-fx-parallax component. (Separate PR to remove parallax)
-Documentation and tests to follow.
Closes #7928
Closes #1443
Closes #7925

/cc @ericlindley-g

honeybadgerdontcare

Validator changes look fine.

honeybadgerdontcare · 2018-01-26T23:37:28Z

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

@@ -0,0 +1,38 @@
+PASS
+|  <!--
+|    Copyright 2016 The AMP HTML Authors. All Rights Reserved.


honeybadgerdontcare · 2018-01-26T23:37:39Z

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

@@ -0,0 +1,37 @@
+<!--
+  Copyright 2016 The AMP HTML Authors. All Rights Reserved.


honeybadgerdontcare · 2018-01-26T23:37:55Z

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

+-->
+<!--
+  Test Description:
+  Tests for the amp-fx-collection tag. See the inline comments.


Remove "See the inline comments." since there aren't any.

honeybadgerdontcare · 2018-01-26T23:38:19Z

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

+|  -->
+|  <!--
+|    Test Description:
+|    Tests for the amp-fx-collection tag. See the inline comments.


Remove "See the inline comments." since there aren't any.

… amp-fx-collection

cvializ · 2018-01-26T23:27:10Z

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

+ * Map of fx type to fx provider class.
+ * @type {Object<FxType, function(new:FxProviderInterface, !../../../src/service/ampdoc-impl.AmpDoc)>}
+ */
+const fxProviders = map();


Can we use computed properties?

const fxProviders = map({ [FxType.PARALLAX]: ParallaxProvider, });

Whoa, TIL: using [] in the key will evaluate it. Yikes?
@jridgewell As our TC-39 resident, you get to see me complain about JS now :).
How is this syntax consolidated with native JS Map that can have object keys? What would var x = new Map({ [Key]: Val }) do? I would have expected key to be an array.

Huh? {[KEY]: VALUE} is called a computed key, so of course it evaluates the key...

const KEY = 'key'; const computed = { [KEY]: 'value', }; assert.deepEquals(computed, { key: 'value' });

yeah but wouldn't using [] to mean compute vs to mean array conflict with native Map that supports any object (including arrays) as keys? How does the new native Map know what I mean by x = new Map({ [Key]: Val }), what if I actually want the array [Key] to be the key for my Val?

Maps take an iterable (so arrays), not an object.

oh, I see. still pretty ugly that [] has a second meaning. Supporting template literals as keys so dev can start migrating over seems like a decent choice, not sure if that is something they are considering

const computed = { `${key}`: 'value' }

The intention is to mirror bracket access:

let obj = {}; obj[some_expresion] = value; // ES6 let obj = { [some_expression]: value, };

cvializ · 2018-01-26T23:31:11Z

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

+    dev().assert(!this.seen_.includes(fxElement));
+    dev().assert(this.viewer_.isVisible());
+
+    /** @type {!Array<!FxType>} */


nit: this /** @type */ is superfluous since Closure can infer the type from the return type of the function below

cvializ · 2018-01-29T17:45:59Z

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

+  scan_() {
+    const fxElements = this.root_.querySelectorAll('[amp-fx]');
+    iterateCursor(fxElements, fxElement => {
+      if (this.seen_.includes(fxElement)) {


Where are items added to this.seen_? should that happen in register_? Or in a subclass?

😰 this is what happens when there are no unit tests.

cvializ · 2018-01-29T17:53:08Z

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

+    const viewportHeight = this.viewport_.getHeight();
+
+    let offsetTop = 0;
+    for (let n = this.element_; n; n = n./*OK*/offsetParent) {


nit: use a more descriptive variable name than n to help readers understand faster

cvializ · 2018-01-29T17:56:21Z

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

+    }
+    const aboveTheFold = (offsetTop < viewportHeight);
+
+    if (aboveTheFold) {


style nit, super optional: convert if/else to ternary for brevity

cvializ · 2018-01-29T18:02:45Z

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

+    this.adjustedViewportHeight_ = this.getAdjustedViewportHeight_();
+
+    // start observing position of the element.
+    this.observePositionChanges_();


I notice that some of your constructors in this PR mutate global state, e.g. adding event listeners, installing services. You can mutate global state, but then you have to stub those functions in tests which makes writing tests more difficult. I think that having pure constructors may make testing easier, and just have the consumers of the class call the state-mutating functions like observePositionChanges().

cvializ · 2018-01-29T18:06:46Z

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

+   */
+  installOn(element) {
+    Services.vsyncFor(this.ampdoc_.win).measure(() => {
+      new ParallaxElement(element, this.viewport_, this.positionObserver_);


It's not obvious why this constructor call is wrapped in a measure. Is it because the constructor calls getAdjustedViewportHeight? If so, I would suggest having getAdjustedViewportHeight return a promise from vsync.measurePromise instead.

cvializ · 2018-01-29T18:13:54Z

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

+    /** @private @const {!Object<FxType, FxProviderInterface>} */
+    this.fxProviderInstances_ = map();
+
+    ampdoc.whenReady().then(() => this.viewer_.whenFirstVisible()).then(() => {


Is the whenReady necessary? Does the viewer ever resolve whenFirstVisible before whenReady is resolved? If either one could happen first, maybe something like this would better communicate your intent.

Promise.all([ ampdoc.whenReady(), this.viewer_.whenFirstVisible(), ]).then(/*...*/)

yeah whenFirstVisible is purely based on a message received from the viewer so I assumed it can happen at any moment. Switched to Promise.all

aghassemi · 2018-01-30T00:25:09Z

@cvializ Thanks, PTAL.

cvializ

LGTM!

cvializ · 2018-01-30T00:32:17Z

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

+    // Translate the element offset pixels.
+    // No need for vsync mutate, position observer only calls back at most
+    // every animation frame.
+    setStyles(this.element_,


I thought the purpose of vsync mutate was not to batch individual components' calls, but to batch mutations across all components that might manipulate the DOM at any moment? Wouldn't you still need it here to coordinate with other AMP components manipulating the DOM? I'm still trying to learn when it is and isn't needed 😅

Your assessment is correct but we still don't need it here since we are only changing transform which is done at composition layer and won't cause relayouts. Updated the comments (it also reminded me to add 'will-change': 'transform' to the element as well) . Using vsyncMutate would make us be one frame behind which doesn't feel as nice.

kk thanks for explaining

* Revision bump for #13036 * Revision bump for #13073 * Add DEDUPE_ON_MINIFY flags to license tags so that release process can reduce the number of identical duplicate licenses in the minified validator. * Add requires_extension to AttrSpec. * JSDoc updates. * Generated validator javascript improvements. * Add comment to ValidationError hinting at how to render. * Revision bump for #12955 * Add new error types for future CSS validation. * Revision bump for #12798 * Fix a typo. * Allow animation-timing-function for keyframes * Fix typo

* Revision bump for ampproject#13036 * Revision bump for ampproject#13073 * Add DEDUPE_ON_MINIFY flags to license tags so that release process can reduce the number of identical duplicate licenses in the minified validator. * Add requires_extension to AttrSpec. * JSDoc updates. * Generated validator javascript improvements. * Add comment to ValidationError hinting at how to render. * Revision bump for ampproject#12955 * Add new error types for future CSS validation. * Revision bump for ampproject#12798 * Fix a typo. * Allow animation-timing-function for keyframes * Fix typo

aghassemi added 7 commits January 25, 2018 11:03

initial setup

a3c286f

more coolness

5beb367

polish

a385972

making PR cleaner

67b82f4

fx collections

0b46c7c

parallax

890e275

parallax

cea97a6

googlebot added the cla: yes label Jan 26, 2018

aghassemi requested review from cvializ, honeybadgerdontcare and cathyxz January 26, 2018 23:14

honeybadgerdontcare approved these changes Jan 26, 2018

View reviewed changes

aghassemi added 2 commits January 26, 2018 15:46

amp-fx-collection: parallax

b0342a6

Merge branch 'amp-fx-collection' of github.com:aghassemi/amphtml into…

b5ce1a9

… amp-fx-collection

aghassemi mentioned this pull request Jan 27, 2018

Remove the old amp-fx-parallax and the experiment flag #13077

Merged

cvializ reviewed Jan 29, 2018

View reviewed changes

Address review comments

b75f341

cvializ approved these changes Jan 30, 2018

View reviewed changes

add will-change

ed2e762

aghassemi merged commit b685a07 into ampproject:master Jan 30, 2018

aghassemi mentioned this pull request Feb 5, 2018

Status Update 2018-02-05 #13235

Closed

Gregable pushed a commit that referenced this pull request Feb 7, 2018

Revision bump for #13073

dc90b2c

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Feb 14, 2018

amp-fx-collection: parallax (ampproject#13073)

897884a

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Feb 14, 2018

amp-fx-collection: parallax (ampproject#13073)

f48df39

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Feb 14, 2018

amp-fx-collection: parallax (ampproject#13073)

e69f87e

RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018

amp-fx-collection: parallax (ampproject#13073)

c9a52f6

protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018

amp-fx-collection: parallax (ampproject#13073)

9bdc2bc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

amp-fx-collection: parallax #13073

amp-fx-collection: parallax #13073

aghassemi commented Jan 26, 2018 •

edited

Loading

honeybadgerdontcare left a comment

honeybadgerdontcare Jan 26, 2018

honeybadgerdontcare Jan 26, 2018

honeybadgerdontcare Jan 26, 2018

honeybadgerdontcare Jan 26, 2018

cvializ Jan 26, 2018

aghassemi Jan 29, 2018

jridgewell Jan 30, 2018

aghassemi Jan 30, 2018

jridgewell Jan 30, 2018

aghassemi Jan 30, 2018

jridgewell Jan 30, 2018

cvializ Jan 26, 2018

aghassemi Jan 30, 2018

cvializ Jan 29, 2018

aghassemi Jan 29, 2018

cvializ Jan 29, 2018

cvializ Jan 29, 2018

cvializ Jan 29, 2018

aghassemi Jan 30, 2018

cvializ Jan 29, 2018

aghassemi Jan 30, 2018

cvializ Jan 29, 2018

aghassemi Jan 29, 2018

aghassemi commented Jan 30, 2018

cvializ left a comment

cvializ Jan 30, 2018

aghassemi Jan 30, 2018

cvializ Jan 30, 2018

		@@ -0,0 +1,37 @@
		<!--
		Copyright 2016 The AMP HTML Authors. All Rights Reserved.

amp-fx-collection: parallax #13073

amp-fx-collection: parallax #13073

Conversation

aghassemi commented Jan 26, 2018 • edited Loading

honeybadgerdontcare 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

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

aghassemi commented Jan 30, 2018

cvializ 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

aghassemi commented Jan 26, 2018 •

edited

Loading