Use IntersectionObserver if available #113

snewcomer · 2017-11-27T05:09:25Z

IntersectionObserver API

This PR provides an easy upgrade path to use IntersectionObserver. This also doesn't have any effect on existing users and will fallback to rAF if not in target browser.

Also added two routes - infinity and infinity-scrollable to perf-test and test.

use IntersectionOverride by default
allow for scrollable area (a configuration option on the IntersectionObserver)
tests (including separate tests for IntersectionObserver, rAF and scrollEvent)
documentation

close #106 #67
Ref prev PR #112
Partial Fix #37

snewcomer · 2017-11-27T05:44:27Z

So this solution still keeps rAF and the scrollEvent code around. ember-spaniel provides an IntersectionObserver polyfill (that doesn't fallback to native if present). But I saw this PR and thought it may be good to look at this sol'n when this is merged. Thoughts? Plus this PR wouldn't require too much of a refactor for the users whereas the IntersectionObserver only would remove a lot of the config variables and be good for a major release....

snewcomer · 2017-11-27T05:45:27Z

addon/mixins/in-viewport.js

-        bind(this, this._setViewportEntered, context)
-      );
+    if (get(this, 'viewportUseIntersectionObserver')) {
+      const { top, left, bottom, right } = this.viewportTolerance;


Maybe can make this a separate config item specific to IntersectionObserver instead of both IntersectionObserver and rAF. Thoughts?

snewcomer · 2017-11-27T05:48:34Z

tests/dummy/app/controllers/infinity.js

+const models = [...Array(10).fill().map(() => `https://s3.amazonaws.com/uifaces/faces/twitter/${images[(Math.random() * images.length) | 0]}/128.jpg`)];
+
+export default Ember.Controller.extend({
+  models,


This route uses optimistic rendering. Guessing we don't want to control this behavior and leave that up to the users? i.e. 2 requests will be made - first for the 1st set of records. While that is pending, it will trigger infinityLoad().

snewcomer · 2017-11-28T21:40:43Z

addon/mixins/in-viewport.js

+    if (get(this, 'viewportUseIntersectionObserver')) {
+      const { top, left, bottom, right } = this.viewportTolerance;
+      const options = {
+        root: null, 


Another question - if null, the root is the layout view (document.documentElement). One case I have run up against before is the scrollable area is not the window but rather a confined area in the viewport.

Would you be in favor of a scrollableArea config variable?

snewcomer · 2017-12-05T18:11:28Z

Hey! Sry to be a bug. A review would be great when (or more apt 'if' :) ) somebody has time!

👀

@poteto @kellyselden @langalex

snewcomer · 2017-12-05T20:13:08Z

addon/mixins/in-viewport.js

    const elementId = get(this, 'elementId');

+    const context = get(this, 'scrollableArea') ? get(this, 'scrollableArea') : window;


Could probably make a variable on this mixin instead of this ternary.

const context = get(this, 'scrollableArea') || window ?

snewcomer · 2017-12-05T20:15:53Z

tests/dummy/app/routes/infinity-scrollable-scrollevent.js

@@ -0,0 +1,4 @@
+import ScrollableRoute from './infinity-scrollable';
+
+export default ScrollableRoute.extend({


Don't necessarily like this, but to avoid duplication and allow for deterministic tests, I separated out each InfinityObserver, rAF, and scrolEvent options in the lib to three separate routes with a scrollable area.

snewcomer · 2017-12-05T20:20:32Z

Also just to note, this PR would benefit ember-infinity (PR - i.e. got it working with this branch) && ember-light-table (related).

BartTK · 2018-01-12T07:12:22Z

I've tested this change on my project and it scrolls a lot smoother then the rAF approach. However I was disappointed with respect to the loading time. When having a few thousands components on a page using this mixin, it takes several seconds to load them. In my specific case, all components used the same scrollableArea and the same options. As a result, I could re-use a single IntersectionObserver instance that observes all components. This greatly improved the loading time.

simonihmig · 2018-01-19T16:41:13Z

Would love to see this land! 👍
Anything missing here? (except for the rebase)

snewcomer · 2018-01-19T21:19:55Z

I'm going to upgrade to 2.18 and use the new testing apis in a separate commit. Getting a test leaking/flakey test.

snewcomer · 2018-01-19T23:05:50Z

So since I hadn't got a review yet, I updated the pkg to 2.18 and changed everything over to the new testing apis and @ember module namespace. Part of it was to fix a flakey test since the qunit container changed from 2.7 to 2.17 and then it snowballed from there 🤦‍♂️

I know it isn't a very nice thing to do from a reviewers POV, so I can easily break the last two commits into another PR if everybody would like. However, since most of main changes are new files, upgrading didn't change much:

Add IntersectionObserver API in in-viewport mixin (existing file)
Add acceptance tests (totally new files).
New routes for testing IntersectionObserver, rAF, and scroll events (totally new files)

cibernox · 2018-01-20T03:53:44Z

@snewcomer is it possible to submit the update to 2.18 in a separate PR?

I haven't reviewed this because I'm not familiar with the codebase. TBH I don't know who is the best person to review it.

snewcomer · 2018-01-23T11:59:43Z

@BartTK great insight btw. Curious, how did you get one IntersectionObserver to rule them all? I’m thinking your scenario might be somewhat interesting....where we could also somehow use one IntersectionObserver to ‘observe’ each element instead of many IntersectionObservers to ‘observe’ one element. Not totally sure of the perf implications, but your observation makes it seem like we should look into this. (after this PR)

BartTK · 2018-01-23T12:48:58Z

You only need a single IntersectionObserver per scrollableArea/options pair. When creating the observer, add it to a static administration. Future components having the same scrollableArea and options can re-use that IntersectionObserver instead of creating a new one. Just call 'observe' again for the new component.

There are a few things to keep in mind. You need to do reference counting to know when to destroy the observer. Furthermore you cannot bind '_onIntersection' to 'this' anymore; a single callback will be used by multiple components. Instead of receiving a single entry, this callback now receives a list of entries. These entries only contain the DOM element, you need to maintain another administration to map that element to its component.

In my case, this resulted in a large performance boost with respect to the loading time of the page.

snewcomer · 2018-01-23T12:54:04Z

@BartTK cool 😎 so you forked and modified this PR then? Once this PR gets in, and of course if you have time, it would be a great PR!

sbatson5 · 2018-01-29T14:04:49Z

README.md

 - `viewportUseRAF: boolean`

  Default: Depends on browser

-  As it's name suggests, if this is `true`, the Mixin will use `requestAnimationFrame` instead of the Ember run loop. Unless you want to force enabling or disabling this, you won't need to override this option.
+  As it's name suggests, if this is `true` and the IntersectionObserver API is not available in the target browser, the Mixin will use `requestAnimationFrame`. Unless you want to force enabling or disabling this, you won't need to override this option.


its instead of it's

sbatson5 · 2018-01-29T14:08:23Z

addon/mixins/in-viewport.js

    const elementId = get(this, 'elementId');

+    const context = get(this, 'scrollableArea') ? get(this, 'scrollableArea') : window;


const context = get(this, 'scrollableArea') || window ?

sbatson5 · 2018-01-29T14:09:49Z

tests/acceptance/infinity-test.js

+
+  andThen(() => {
+    assert.equal(findAll('.infinity-svg-scrollEvent').length, 20);
+    // assert.equal(find('.infinity-scrollable-scrollEvent.inactive').length, 1, 'component is inactive after fetching more data');


Commented out test

sbatson5 · 2018-01-29T14:13:05Z

tests/dummy/app/routes/infinity-scrollable.js

+    return [...Array(10).fill().map(() => `${images[(Math.random() * images.length) | 0]}`)];
+  },
+  setupController(controller, model) {
+    set(controller, 'model', model);


Isn't this the default behavior of setupController? Is there a reason to overwrite?

sbatson5 · 2018-01-29T14:13:52Z

tests/dummy/app/templates/infinity-scrollable.hbs

+<div class="list" style="height: 500px; overflow: scroll;">
+  <ul>
+    {{#each model as |val|}}
+    <div class="infinity-svg">


Indent? Only to be consistent with the next file where the each block is indented

Duder-onomy · 2018-02-02T23:22:44Z

BOOM!

snewcomer force-pushed the intersection-observer branch 2 times, most recently from d68b0a3 to 9b3fd6e Compare November 27, 2017 05:43

snewcomer commented Nov 27, 2017

View reviewed changes

snewcomer commented Nov 28, 2017

View reviewed changes

snewcomer mentioned this pull request Nov 29, 2017

Replace with ember-in-viewport adopted-ember-addons/ember-infinity#242

Merged

2 tasks

snewcomer changed the title ~~Use IntersectionObserver if set to true~~ Use IntersectionObserver if available Dec 1, 2017

snewcomer commented Dec 5, 2017

View reviewed changes

This was referenced Dec 19, 2017

Use #ember-testing-container as a context when testing #116

Closed

Look into replacing/modifying our infinity-loader with ember-in-viewport adopted-ember-addons/ember-infinity#240

Closed

snewcomer mentioned this pull request Jan 13, 2018

Load previous pages adopted-ember-addons/ember-infinity#183

Closed

snewcomer force-pushed the intersection-observer branch 2 times, most recently from 3765ea4 to 463774b Compare January 19, 2018 13:33

snewcomer mentioned this pull request Jan 19, 2018

Ember 2.17 #111

Merged

1 task

simonihmig mentioned this pull request Jan 19, 2018

Remove jQuery #118

Closed

snewcomer force-pushed the intersection-observer branch 2 times, most recently from 44e4ce4 to 5742f46 Compare January 19, 2018 20:49

snewcomer requested a review from cibernox January 19, 2018 21:00

snewcomer self-assigned this Jan 19, 2018

snewcomer added the enhancement label Jan 19, 2018

snewcomer force-pushed the intersection-observer branch 2 times, most recently from 5752a85 to 04267ec Compare January 19, 2018 22:28

snewcomer force-pushed the intersection-observer branch from fa83816 to 45b69f3 Compare January 20, 2018 14:07

Use IntersectionObserver

30e8aa8

snewcomer force-pushed the intersection-observer branch from 8a2a0f4 to 30e8aa8 Compare January 20, 2018 14:25

snewcomer mentioned this pull request Jan 28, 2018

Investigate use of one Intersection Observer #119

Closed

sbatson5 reviewed Jan 29, 2018

View reviewed changes

update based on comments

a62996d

snewcomer mentioned this pull request Feb 1, 2018

Change in viewport without scroll don't trigger didEnterViewport #120

Closed

snewcomer added 2 commits February 1, 2018 17:38

fixup scroll event test

4dc7878

more stable tests w/ waitFor

5ff221a

bcardarella approved these changes Feb 2, 2018

View reviewed changes

bcardarella merged commit b0de191 into DockYard:master Feb 2, 2018

snewcomer deleted the intersection-observer branch July 19, 2018 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use IntersectionObserver if available #113

Use IntersectionObserver if available #113

snewcomer commented Nov 27, 2017 •

edited

Loading

snewcomer commented Nov 27, 2017 •

edited

Loading

snewcomer Nov 27, 2017 •

edited

Loading

snewcomer Nov 27, 2017 •

edited

Loading

snewcomer Nov 28, 2017

snewcomer commented Dec 5, 2017 •

edited

Loading

snewcomer Dec 5, 2017

sbatson5 Jan 29, 2018

snewcomer Dec 5, 2017

snewcomer commented Dec 5, 2017 •

edited

Loading

BartTK commented Jan 12, 2018

simonihmig commented Jan 19, 2018

snewcomer commented Jan 19, 2018

snewcomer commented Jan 19, 2018 •

edited

Loading

cibernox commented Jan 20, 2018

snewcomer commented Jan 23, 2018

BartTK commented Jan 23, 2018

snewcomer commented Jan 23, 2018

sbatson5 Jan 29, 2018

sbatson5 Jan 29, 2018

sbatson5 Jan 29, 2018

sbatson5 Jan 29, 2018

sbatson5 Jan 29, 2018

Duder-onomy commented Feb 2, 2018

		const elementId = get(this, 'elementId');

		const context = get(this, 'scrollableArea') ? get(this, 'scrollableArea') : window;

		@@ -0,0 +1,4 @@
		import ScrollableRoute from './infinity-scrollable';

		export default ScrollableRoute.extend({

Use IntersectionObserver if available #113

Use IntersectionObserver if available #113

Conversation

snewcomer commented Nov 27, 2017 • edited Loading

IntersectionObserver API

snewcomer commented Nov 27, 2017 • edited Loading

snewcomer Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

snewcomer Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented Dec 5, 2017 • edited Loading

BartTK commented Jan 12, 2018

simonihmig commented Jan 19, 2018

snewcomer commented Jan 19, 2018

snewcomer commented Jan 19, 2018 • edited Loading

cibernox commented Jan 20, 2018

snewcomer commented Jan 23, 2018

BartTK commented Jan 23, 2018

snewcomer commented Jan 23, 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

Duder-onomy commented Feb 2, 2018

snewcomer commented Nov 27, 2017 •

edited

Loading

snewcomer commented Nov 27, 2017 •

edited

Loading

snewcomer Nov 27, 2017 •

edited

Loading

snewcomer Nov 27, 2017 •

edited

Loading

snewcomer commented Dec 5, 2017 •

edited

Loading

snewcomer commented Dec 5, 2017 •

edited

Loading

snewcomer commented Jan 19, 2018 •

edited

Loading