Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow programmatic control of observation #397

Merged
merged 2 commits into from
Jun 28, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Allow programmatic control of observation
Add a named new argument, `isObserving`, with a default value of
`true`, which users can pass to explicitly opt into or out of observing
an element at a given time. This allows for a declarative invocation of
the modifier itself based on tracked state, rather than having to handle
it in the callback(s) passed to the modifier.

Users can work around this today by only passing the `onEnter` or
`onExit` callbacks conditionally:

    <div
      {{did-intersect
        onEnter=(if this.shouldDoSomething this.doSomething no-op)
        onExist=(if this.shouldDoSomething this.doSomethingElse no-op)
      }}
    >

However, as this example shows, this is repetive (and potentially
error- prone). The new functionality allows a less error-prone, less
repetitive, more expressive approach:

    <div
      {{did-intersect
      	isObserving=this.shouldDoSomething
        onEnter=this.doSomething
        onExit=this.doSomethingElse
      }}
    >

Resolves #396
  • Loading branch information
chriskrycho committed Jun 24, 2021
commit a70d1cd4ed2efb1acabc868d7aba77d53ad7d212
15 changes: 13 additions & 2 deletions addon/modifiers/did-intersect.js
Original file line number Diff line number Diff line change
@@ -77,7 +77,14 @@ export default class DidIntersectModifier extends Modifier {
return;
}

let { onEnter, onExit, maxEnter, maxExit, options } = this.args.named;
let {
onEnter,
onExit,
maxEnter,
maxExit,
options,
isObserving = true,
} = this.args.named;

assert('onEnter or/and onExit is required', onEnter || onExit);

@@ -161,7 +168,11 @@ export default class DidIntersectModifier extends Modifier {
this._options = options;
}

this.observe();
if (isObserving) {
this.observe();
} else {
this.unobserve();
}
}

// Move to willDestroy when we want to drop support for versions below ember-modifier 2.x
58 changes: 54 additions & 4 deletions tests/integration/modifiers/did-intersect-test.js
Original file line number Diff line number Diff line change
@@ -16,13 +16,21 @@ module('Integration | Modifier | did-intersect', function (hooks) {
this._admin = {};
}

observe = sinon.stub();
unobserve = sinon.stub();
observe = sinon.stub().callsFake(() => {
this.isObserving = true;
});
unobserve = sinon.stub().callsFake(() => {
this.isObserving = false;
});
addEnterCallback = sinon.stub().callsFake((element, callback) => {
this.onEnterCallback = sinon.spy(callback);
this.onEnterCallback = sinon.spy(() => {
if (this.isObserving) callback();
});
});
addExitCallback = sinon.stub().callsFake((element, callback) => {
this.onExitCallback = sinon.spy(callback);
this.onExitCallback = sinon.spy(() => {
if (this.isObserving) callback();
});
});
}

@@ -344,4 +352,46 @@ module('Integration | Modifier | did-intersect', function (hooks) {

assert.equal(this.newExitStub.callCount, 1, 'new exit callback is called');
});

module('modifier accepts `isObserving` argument', function () {
test('with a truth(y) value', async function (assert) {
assert.expect(2);
Copy link
Owner

@elwayman02 elwayman02 Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer not to use assert.expect for tests where the assertions are always called synchronously rather than in callbacks. This makes the tests brittle to change without adding a lot of value, in my experience.

(Yes, I know a few of those snuck through above. I missed them in previous reviews :( )

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can fix this in another PR, since it needs to be updated everywhere.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #398

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, I am 100% in agreement; I added it solely to match the existing tests I saw. 😅


await render(hbs`
<div
{{did-intersect
isObserving=true
onEnter=this.enterStub
onExit=this.exitStub
}}
></div>
`);

this.observerManagerMock.onEnterCallback();
this.observerManagerMock.onExitCallback();

assert.ok(this.enterStub.calledOnce, 'the enter callback is invoked');
assert.ok(this.exitStub.calledOnce, 'the onExit callback is invoked');
});

test('with a false(y) value', async function (assert) {
assert.expect(2);

await render(hbs`
<div
{{did-intersect
isObserving=false
onEnter=this.enterStub
onExit=this.exitStub
}}
></div>
`);

this.observerManagerMock.onEnterCallback();
this.observerManagerMock.onExitCallback();

assert.ok(this.enterStub.notCalled, 'the enter callback is not invoked');
assert.ok(this.exitStub.notCalled, 'the onExit callback is not invoked');
});
});
});