Implement parallax effect extension by cvializ · Pull Request #7794 · ampproject/amphtml

cvializ · 2017-02-24T23:19:18Z

Implements parallax effect extension with tests and usage markdown
Based on @aghassemi's parallax deep dive demos

aghassemi · 2017-02-25T00:20:18Z

@cvializ I will take a look at soon, meanwhile could you please experiment flag around the whole thing? (amp-fx-parallax as the experiment name)

cvializ · 2017-02-25T00:48:09Z

Thanks! For the experiment flag, is the guard at extensions/amp-fx-parallax/0.1/amp-fx-parallax.js#164 enough? It runs when the extension script is included.

aghassemi · 2017-02-25T00:50:16Z

@cvializ almost, needs an entry in experiments.js as well

aghassemi

Some requests for structural changes. Will review the details of the implementation after the structure is changed.

aghassemi · 2017-02-27T22:08:02Z

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

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


Let's add a article-parallax.amp.html to examples folder.
Maybe: https://gist.github.com/aghassemi/0fde9b1df0ad531dbdfc29588ca27c73 (paths need updating)

aghassemi · 2017-02-27T22:08:19Z

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

+    return new ParallaxService(win);
+  }).start_();
+};
+


move the experiment assert right before installing the service here.

aghassemi · 2017-02-27T22:19:44Z

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

+ * Installs parallax handlers, tracks the previous scroll position and
+ * implements post-parallax-update scroll hooks.
+ */
+export class ParallaxService {


let's move the service under service folder and make it an "ampdoc" level service rather than a window service. See video-manager.js and video-manager-impl for an example.

Note that moving this outside of the component does NOT mean the core AMP library (v0.js) grows in size because it would only get bounded into binaries that actually use it ( e.g. amp-fx-parallax/0.1/amp-fx-parallax.js)

aghassemi · 2017-02-27T22:23:51Z

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

+   * Install handlers after the document has loaded.
+   */
+  start_() {
+    const win = this.win_;


move to or call from the constructor. installing a service should be enough to get it going.

aghassemi · 2017-02-27T22:44:12Z

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

+ * in the document.
+ * @param {!Window} global
+ */
+function installParallaxHandlers(global) {


let's move this, parallaxMutate and transform to be private methods on ParallaxService class and make all other methods on ParallaxService also private.

aghassemi

Awesome! Just a few nits and then we can merge.

aghassemi · 2017-03-01T17:29:13Z

examples/article-parallax.amp.html

+        <span>Lorem Ipsum Dolor Sit Amet Consectetur Adipiscing<span>
+      </div>
+    </h1>
+    <amp-img height="50vh" layout="fixed-height" src="http://localhost:8000/examples/img/hero@1x.jpg"></amp-img>


keep src relative

aghassemi · 2017-03-01T17:30:52Z

examples/article-parallax.amp.html

+<noscript>
+  <style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style>
+</noscript>
+<script async custom-element="amp-fx-parallax" src="../../dist/v0/amp-fx-parallax-0.1.max.js"></script>


update the paths to point to CDN (to test locally, hit article-parallax.amp.max.html (note .max.) and server will rewrite the paths to local files.

aghassemi · 2017-03-01T17:32:57Z

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

+import {ampdocServiceFor} from '../../../src/ampdoc';
+import {installParallaxForDoc} from '../../../src/service/parallax-impl';
+
+const ampdoc = ampdocServiceFor(AMP.win).getAmpDoc();


wrap these in onDocumentReady

aghassemi · 2017-03-01T17:43:55Z

extensions/amp-fx-parallax/amp-fx-parallax.md

+
+The factor to use when scrolling. A value greater than 1 scrolls the element upward when the user scrolls down the page. A value less than 1 scrolls the element downward when the user scrolls downward. A value of 1 behaves normally. A value of 0 effectively makes the element scroll fixed with the page.
+
+## Styling


I don't think we need a section regarding styling since it is a purely a behaviour attribute.

aghassemi · 2017-03-01T17:44:36Z

src/service/parallax-impl.js

+   * Get the previous scroll value.
+   * @return {number}
+   */
+  getPreviousScroll() {


this and the following 5 methods can become private too

aghassemi · 2017-03-01T17:50:07Z

also add the component to the whitelist under dep-check-config under Rules for extensions. to fix the pre-check build error

aghassemi · 2017-03-01T17:50:25Z

@muxin Yuxi, let's wait for this for the Canary cut as well. Thanks!

cvializ · 2017-03-01T18:11:18Z

build-system/dep-check-config.js

          'src/service/video-manager-impl.js',
      'extensions/amp-a4a/0.1/amp-a4a.js->src/service/variable-source.js',
+      'extensions/amp-fx-parallax/0.1/amp-fx-parallax.js->' +
+          'src/service/parallax-impl.js',


* initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * fixed linting issues (#7673) * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * fixed linting issues (#7673) * nexxtv player review fixes (#7816) * nexxtv player: updated validation rules (#7816) * nexxtv player validator ordered alphabetical, changed spec_url * fire amp-dom-update event on insert and replace (#7819) * temp * trigger `amp-dom-update` event on insert and replace * switch -amp-form to i-amp-form * Validator Rollup (#7844) * Revision bump. * Revision bump for amp-playbuzz changes in #7450 * Revision bump. * Allow filtering by HtmlFormat (AMP, AMP4ADS) in the code generator. * Revision bump. * Revision bump due to Github pull. * Code generation now driven by a variable LIGHT, which is broader than the previous GENERATE_DETAILED_ERRORS distinction. LIGHT implies filtered for specific format (AMP or AMP4ADS, and only amp.validator.validateSaxEvents, and no detailed errors. * Generate ValidatorRules.directAttrLists and globalAttrs and ampLayoutAttrs, reducing overhead by indirection. * Implement parallax effect extension (#7794) * Do not set hidden attribute on hide/collapse (#7879) * amp-bind: Support `Math` functions (#7797) * initial commit for Math in amp-bind * fix lint * ali's comment * avoid scope conflicts with built-in functions * remove Math functions from documentation * Measure 3P ad latency (#7902) * Update spec URLs from github to ampproject. (#7901) * Update github urls to ampproject urls * test .out updated * Too many slashes. * Moved Developing.md and Design_Principles.md (#7908) * Rename DEVELOPING.md to contributing/DEVELOPING.md * Move files to new folder * Renaming toggle to toggleVisibility because it was conflicting with sidebar and breaking it (#7855) * disable scrollRestoration auto for embed case (#7899) * Disable Fast Fetch for all ads when remote.html is used. (#7906) * Fix DoubleClick Fast Fetch bugs around categoryExclusions and tagForChildDirectedTreatment (#7843) * Fix incorrect parameter name for `tagForChildDirectedTreatment` in DoubleClick. * Added unit test for tagForChildDirectedTreatment. * Fixed the categoryExclusions bug in Fast Fetch DoubleClick. * Removed parens in arrow functions to satisfy linter * Adds two new experiment IDs (#7850) * Adds two new experiment IDs to distinguish "any externally triggered" experiment from "any internally triggered". Also updates tests to remove a number of assumptions that only a single eid will be populated. * Use return val to detect "externally selected"; test updates. * Updated network implementation guide. (#7862) * Updated network impl guide. * Changed 'validation' to 'verification' * Cid timeout error should not be dev error (#7911) * Fix amp-ad test (#7918) * Update amp-install-serviceworker.md (#7932) Fully escape javascript regex. * Update amp-cache-modifications.md (#7933) Add `data-no-service-worker-fallback-shell-url` as a URL to be rewritten as absolute. * amp-fx-parallax Don't use global (#7942) * Viewer integration: rewrite error message to indicate request name (#7923) * rewrite error message * update * ticks * Fix up links in DEVELOPING.md (#7944) We moved DEVELOPING.md to the contributing folder but didn't update many of the relative links accordingly. * Popin ad extension document updated. (#7674) * Add popin ad extension. * register popin. * Add resizeable attribute. * Remove resizable. * Implemented the render-start and no-content APIs. * Fixed lint. * Remove quatation. * Use tei@popin.cc * Rebase old commit . * Fixed document because tag was not closed. * I replaced the removed double quotes. * Add double quotes. * Support for bind expressions in mustache templates (#7602) * first pass at adding/removing subtrees * tests passing * Add ability to add and remove bindings in the subtree rooted at a specific node * lint fixes * merge conflict * ci issues * some cleanup * pr comments * pr comments, new method of removing bindings for node and its children * fix some lint warnings * test and lint fixes * mutation observer * update comments * add observer * naive implementation * cleanup * clean * cleanup bind impl * very simple example of bind in a template * pr comments * lint errors * pr comments * cleanup * lint errors * pr comments * pr comments and lint warnings * typo * comments * more lint warnings * pr comment * change README * cleanup * pr comments * pr comments 2 * lint warnings * don't use foreach on Nodelist * casting * more ci warnings * even more ci warnings * even more ci warnings * even more ci issues * even more ci comments * even more ci issues * pr comments * more pr comments * fix refactoring oversight * pr comments * pr comments * merge * pr comments * ci issues * pr comments * assertElement * pr comments * typo * updating sanitizer * fixing up sanitizer test * pr comments * pr comments * expanded on amp-mustache tests * pr comments * remove class from whitelist * restore whole sandbox * update mustache validator test and output * pr comments * nexxtv player gulp check-types fixes (#7816) * nexxtv player fix error gulp presumbit with postmessage (#7816) * added dependency check for nexxtv player (#7816) * nexxtv player minor fix in validator (#7816) * nexxtv player fixed validator (#7816) * fixed validator (#7816)

jridgewell · 2017-03-10T21:26:48Z

src/service/parallax-impl.js

+    const previousScrollTop = this.getPreviousScroll_();
+    const delta = previousScrollTop - newScrollTop;
+
+    elements.forEach(element => {


This should be done in a for loop, since scroll performance is critical.

jridgewell · 2017-03-10T21:27:28Z

src/service/parallax-impl.js

+        return;
+      }
+      element.update(delta);
+      this.setPreviousScroll_(newScrollTop);


Why is this done in the loop?

jridgewell · 2017-03-10T21:28:16Z

src/service/parallax-impl.js

+   * @return {string}
+   */
+  transform_(position) {
+    return `translate3d(0,${position.toFixed(2)}px,0)`;


Nit: translateY

jridgewell · 2017-03-10T21:29:18Z

src/service/parallax-impl.js

+        this.parallaxMutate_.bind(this, parallaxElements, viewport);
+
+    viewport.onScroll(() => vsync.mutate(mutate));
+    mutate(); // initialize the elements with the current scroll position


This should be inside a vsync mutate, too.

jridgewell · 2017-03-10T21:30:47Z

src/service/parallax-impl.js

+    this.element_ = element;
+
+    /** @private @const {!function(number):string} */
+    this.transform_ = transform;


Why are we passing this in? Couldn't this just be on the ParallaxElement class?

jridgewell · 2017-03-10T21:33:35Z

src/service/parallax-impl.js

+    const elementRect = viewport.getLayoutRect(this.element_);
+    elementRect.top -= viewportRect.top;
+    elementRect.bottom = elementRect.top + elementRect.height;
+    return this.isRectInView_(elementRect, viewportRect.height);


We have a helper function that does exactly this.

* initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * fixed linting issues (ampproject#7673) * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * nexxtv player changed indents from 4 spaces to 2 * added unlayoutCallback for nexxtv player * initial setup for extension nexxtv-player * added nexxtv player logic * nexxtv player bugfixes, cleanup, tests, docs * added video interface, changed attribute start to seek-to * reworked video interface for nexxtv-player * added message handler for nexxtv player and tests * nexxtv player minor fixes & cleanup * fixed linting issues (ampproject#7673) * nexxtv player review fixes (ampproject#7816) * nexxtv player: updated validation rules (ampproject#7816) * nexxtv player validator ordered alphabetical, changed spec_url * fire amp-dom-update event on insert and replace (ampproject#7819) * temp * trigger `amp-dom-update` event on insert and replace * switch -amp-form to i-amp-form * Validator Rollup (ampproject#7844) * Revision bump. * Revision bump for amp-playbuzz changes in ampproject#7450 * Revision bump. * Allow filtering by HtmlFormat (AMP, AMP4ADS) in the code generator. * Revision bump. * Revision bump due to Github pull. * Code generation now driven by a variable LIGHT, which is broader than the previous GENERATE_DETAILED_ERRORS distinction. LIGHT implies filtered for specific format (AMP or AMP4ADS, and only amp.validator.validateSaxEvents, and no detailed errors. * Generate ValidatorRules.directAttrLists and globalAttrs and ampLayoutAttrs, reducing overhead by indirection. * Implement parallax effect extension (ampproject#7794) * Do not set hidden attribute on hide/collapse (ampproject#7879) * amp-bind: Support `Math` functions (ampproject#7797) * initial commit for Math in amp-bind * fix lint * ali's comment * avoid scope conflicts with built-in functions * remove Math functions from documentation * Measure 3P ad latency (ampproject#7902) * Update spec URLs from github to ampproject. (ampproject#7901) * Update github urls to ampproject urls * test .out updated * Too many slashes. * Moved Developing.md and Design_Principles.md (ampproject#7908) * Rename DEVELOPING.md to contributing/DEVELOPING.md * Move files to new folder * Renaming toggle to toggleVisibility because it was conflicting with sidebar and breaking it (ampproject#7855) * disable scrollRestoration auto for embed case (ampproject#7899) * Disable Fast Fetch for all ads when remote.html is used. (ampproject#7906) * Fix DoubleClick Fast Fetch bugs around categoryExclusions and tagForChildDirectedTreatment (ampproject#7843) * Fix incorrect parameter name for `tagForChildDirectedTreatment` in DoubleClick. * Added unit test for tagForChildDirectedTreatment. * Fixed the categoryExclusions bug in Fast Fetch DoubleClick. * Removed parens in arrow functions to satisfy linter * Adds two new experiment IDs (ampproject#7850) * Adds two new experiment IDs to distinguish "any externally triggered" experiment from "any internally triggered". Also updates tests to remove a number of assumptions that only a single eid will be populated. * Use return val to detect "externally selected"; test updates. * Updated network implementation guide. (ampproject#7862) * Updated network impl guide. * Changed 'validation' to 'verification' * Cid timeout error should not be dev error (ampproject#7911) * Fix amp-ad test (ampproject#7918) * Update amp-install-serviceworker.md (ampproject#7932) Fully escape javascript regex. * Update amp-cache-modifications.md (ampproject#7933) Add `data-no-service-worker-fallback-shell-url` as a URL to be rewritten as absolute. * amp-fx-parallax Don't use global (ampproject#7942) * Viewer integration: rewrite error message to indicate request name (ampproject#7923) * rewrite error message * update * ticks * Fix up links in DEVELOPING.md (ampproject#7944) We moved DEVELOPING.md to the contributing folder but didn't update many of the relative links accordingly. * Popin ad extension document updated. (ampproject#7674) * Add popin ad extension. * register popin. * Add resizeable attribute. * Remove resizable. * Implemented the render-start and no-content APIs. * Fixed lint. * Remove quatation. * Use tei@popin.cc * Rebase old commit . * Fixed document because tag was not closed. * I replaced the removed double quotes. * Add double quotes. * Support for bind expressions in mustache templates (ampproject#7602) * first pass at adding/removing subtrees * tests passing * Add ability to add and remove bindings in the subtree rooted at a specific node * lint fixes * merge conflict * ci issues * some cleanup * pr comments * pr comments, new method of removing bindings for node and its children * fix some lint warnings * test and lint fixes * mutation observer * update comments * add observer * naive implementation * cleanup * clean * cleanup bind impl * very simple example of bind in a template * pr comments * lint errors * pr comments * cleanup * lint errors * pr comments * pr comments and lint warnings * typo * comments * more lint warnings * pr comment * change README * cleanup * pr comments * pr comments 2 * lint warnings * don't use foreach on Nodelist * casting * more ci warnings * even more ci warnings * even more ci warnings * even more ci issues * even more ci comments * even more ci issues * pr comments * more pr comments * fix refactoring oversight * pr comments * pr comments * merge * pr comments * ci issues * pr comments * assertElement * pr comments * typo * updating sanitizer * fixing up sanitizer test * pr comments * pr comments * expanded on amp-mustache tests * pr comments * remove class from whitelist * restore whole sandbox * update mustache validator test and output * pr comments * nexxtv player gulp check-types fixes (ampproject#7816) * nexxtv player fix error gulp presumbit with postmessage (ampproject#7816) * added dependency check for nexxtv player (ampproject#7816) * nexxtv player minor fix in validator (ampproject#7816) * nexxtv player fixed validator (ampproject#7816) * fixed validator (ampproject#7816)

cvializ requested review from aghassemi and dvoytenko February 24, 2017 23:20

cvializ force-pushed the cv-parallax branch from 16cde88 to 2186aaf Compare February 24, 2017 23:54

cvializ force-pushed the cv-parallax branch from 2186aaf to 0eabb16 Compare February 25, 2017 01:22

Implement parallax effect extension.

2804617

cvializ force-pushed the cv-parallax branch from 0eabb16 to 2804617 Compare February 27, 2017 17:43

cvializ mentioned this pull request Feb 27, 2017

Weekly Status 2017-02-27 #7813

Closed

aghassemi suggested changes Feb 27, 2017

View reviewed changes

cvializ force-pushed the cv-parallax branch from 85cbdf5 to e81f674 Compare March 1, 2017 16:45

Factor out the ParallaxService class into services.

10fe57d

cvializ force-pushed the cv-parallax branch from e81f674 to 10fe57d Compare March 1, 2017 16:46

Add parallax article example from @aghassemi

787b479

aghassemi suggested changes Mar 1, 2017

View reviewed changes

Fix nits in review feedback

c83b4ad

cvializ commented Mar 1, 2017

View reviewed changes

aghassemi approved these changes Mar 1, 2017

View reviewed changes

aghassemi merged commit dda8347 into ampproject:master Mar 1, 2017

cvializ deleted the cv-parallax branch March 1, 2017 21:16

cvializ mentioned this pull request Mar 9, 2017

Weekly Status 2017-03-06 #7995

Closed

jridgewell reviewed Mar 10, 2017

View reviewed changes

kmh287 pushed a commit to kmh287/amphtml that referenced this pull request Mar 13, 2017

Implement parallax effect extension (ampproject#7794)

76ce6fb

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Implement parallax effect extension (ampproject#7794)

fea6f7e


		The factor to use when scrolling. A value greater than 1 scrolls the element upward when the user scrolls down the page. A value less than 1 scrolls the element downward when the user scrolls downward. A value of 1 behaves normally. A value of 0 effectively makes the element scroll fixed with the page.

		## Styling

Conversation

cvializ commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aghassemi commented Feb 25, 2017

Uh oh!

cvializ commented Feb 25, 2017

Uh oh!

aghassemi commented Feb 25, 2017

Uh oh!

aghassemi 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!

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!

aghassemi 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi commented Mar 1, 2017

Uh oh!

aghassemi commented Mar 1, 2017

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cvializ commented Feb 24, 2017 •

edited

Loading