Fixes #9

[dminkovsky]

Added some tests. Perhaps they are too much?

[wraithgar]

Probably should have an if (cb) conditional like the other calls to cb.

[wraithgar]

Probably should have an if (cb) conditional like the other calls to cb.

[dminkovsky]

That code path comes via

ampersand-view-switcher/ampersand-view-switcher.js

Line 28 in bc49c6d

this._hide(prev, function () {

, so the cb is passed in and mandatory (async)

[dminkovsky]

That code path comes via

ampersand-view-switcher/ampersand-view-switcher.js

Line 28 in bc49c6d

this._hide(prev, function () {

, so the cb is passed in and mandatory (async)

[wraithgar]

If waitForRemove is falsey _hide is called sans callback

ampersand-view-switcher/ampersand-view-switcher.js

Line 37 in bc49c6d

this._hide(prev);

[wraithgar]

If waitForRemove is falsey _hide is called sans callback

ampersand-view-switcher/ampersand-view-switcher.js

Line 37 in bc49c6d

this._hide(prev);

[dminkovsky]

Yeah, you're right. If customHide has length === 3 even though waitForRemove is false in the config, hide() is called without the cb and then it'll fail. I'll add the if (), thanks.

[dminkovsky]

Yeah, you're right. If customHide has length === 3 even though waitForRemove is false in the config, hide() is called without the cb and then it'll fail. I'll add the if (), thanks.

[wraithgar]

code looks good, the tests read more like you're enforcing things under the hood rather than expected overarching behavior (i.e. how many parameters hide gets?)

[wraithgar]

code looks good, the tests read more like you're enforcing things under the hood rather than expected overarching behavior (i.e. how many parameters hide gets?)

[dminkovsky]

@wraithgar Thanks for your feedback. What do you think now? The changes I made test the public API, which to me includes the params that your passed-in functions get. So I am testing that options.hide gets first the previous view, then the current view. And the expected behavior.

[dminkovsky]

@wraithgar Thanks for your feedback. What do you think now? The changes I made test the public API, which to me includes the params that your passed-in functions get. So I am testing that options.hide gets first the previous view, then the current view. And the expected behavior.

[dminkovsky]

Added this to have a bit less repetition in the setups.

[dminkovsky]

Added this to have a bit less repetition in the setups.

[dminkovsky]

Added this test because it didn't fail, even before the proposed code change in the next commit.

[dminkovsky]

Added this test because it didn't fail, even before the proposed code change in the next commit.

[wraithgar]

lgtm

[wraithgar]

lgtm

[latentflip]

Okay, given that there's another pull request for this already with the same implementation and no tests, and we're seeing reported issues for this in gitter, I think we should merge asap. I have the pull request updated to the latest master with fixed merge commits and passing tests, shall I do it?

[latentflip]

Okay, given that there's another pull request for this already with the same implementation and no tests, and we're seeing reported issues for this in gitter, I think we should merge asap. I have the pull request updated to the latest master with fixed merge commits and passing tests, shall I do it?

[latentflip]

Merged, thanks @dminkovsky and sorry for the delay!

[latentflip]

Merged, thanks @dminkovsky and sorry for the delay!

[latentflip]

Released as 1.1.2

[latentflip]

Released as 1.1.2