-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix #1567 and add tests to cover getChildView taking a model #1619
Conversation
@@ -231,6 +231,9 @@ Marionette.CollectionView = Marionette.View.extend({ | |||
// Retrieve the childView class, either from `this.options.childView` | |||
// or from the `childView` in the object definition. The "options" | |||
// takes precedence. | |||
// This method receives the model that will be passed to the instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to wrap childView in
for docco
@@ -141,6 +142,12 @@ describe('collection view', function() { | |||
it('should trigger "childview:render" for each item in the collection', function() { | |||
expect(this.childViewRender.callCount).to.equal(2); | |||
}); | |||
|
|||
it('should call "getChildView" for each item in the collection', function() { | |||
expect(this.collectionView.getChildView).to.have.been.calledTwice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ expect(this.collectionView.getChildView).to.have.been.calledTwice.and.calledWith(this.collection.models[0], this.collection.models[1])
- expect(this.collectionView.getChildView).to.have.been.calledTwice;
- expect(this.collectionView.getChildView).to.have.been.calledWith(this.collection.models[0]);
- expect(this.collectionView.getChildView).to.have.been.calledWith(this.collection.models[1]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the docs are a bit vague but this might work...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, but this does
expect(this.collectionView.getChildView).to.have.been.calledTwice.
and.calledWith(this.collection.models[0]).
and.calledWith(this.collection.models[1]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YESSS 👍
@@ -1024,10 +1031,12 @@ describe('collection view', function() { | |||
|
|||
describe('when returning the view from addChild', function() { | |||
beforeEach(function() { | |||
this.model = new Backbone.Model({foo: 'bar'}); | |||
this.model = new Backbone.Model({foo: 'bar', view: this.ChildView }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of changing the existing test we should add another one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since getChildView is going to be called either way, should I just write an assertion for that instead?
Nevermind, ignore me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems weird to add it to the model though. Would this not be better:
- function(model) {
- return model.get('view');
- }
+ sinon.stub().returns(this.view);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, not sure I like adding the view to the model
👍 |
Fix #1567 and add tests to cover getChildView taking a model
fixes #1567