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.

Sign up for GitHub

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

Jump to bottom

Trigger itemView.onShow in appendBuffer if collectionView is already shown #866

Merged
merged 1 commit into from
Jan 14, 2014
Merged

Trigger itemView.onShow in appendBuffer if collectionView is already shown #866

merged 1 commit into from
Jan 14, 2014

Conversation

andrewhubbs
Copy link
Contributor

samccone
samccone reviewed Jan 14, 2014
View reviewed changes
src/marionette.collectionview.js
if (this._isShown) {
_.each(this._bufferedChildren, function (child) {
Marionette.triggerMethod.call(child, "show");
});
Copy link
Member

Choose a reason for hiding this comment

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

perhaps after this loop we should set
this._bufferedChildren = [];

to prevent any kind of memory issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to put the reset code there. I chose to leave it with the reset for the buffer itself for parity. It probably makes more sense to immediately reset it though.

@samccone
Copy link
Member

can you change the first line of the commit message to just be fixes collectionView buffer bug

@andrewhubbs
Copy link
Contributor Author

Yep. I'll get the hang of these commit messages eventually...

@samccone
Copy link
Member

hmm looks like this does not handle the case when you have a compositeView you render it and show it... and then reset its collection. (the itemView onShow) never gets called again

@andrewhubbs
Copy link
Contributor Author

Hmm. Ok. I'll look into that.

@andrewhubbs
Copy link
Contributor Author

Yeah sorry. I didn't see the composite view overrides. Fixing now.

@samccone
Copy link
Member

ah nothing to be sorry about, really appreciate you jumping in and helping out on this one.

@andrewhubbs
Copy link
Contributor Author

Updated.

Has any thought been put into placing a getItemViewContainer function on CollectionView? The difference between the rendering code basically comes down to that. Now that the default implementation of appendHtml is handling more than dumping content into the view element and appendBuffer duplicates the append logic it might make sense to do something like:

Marionette.CollectionView.prototype.getItemViewContainer : function () { return this.$el; }

If we did, then the implementation of appendHtml and appendBuffer could be shared.

@andrewhubbs
fixes collectionView buffer bug
4e2642e 
* Fixes marionettejs#865
* Trigger itemView.onShow in appendBuffer
* Fix bug in collectionView.spec
@samccone
Copy link
Member

nice!

samccone added a commit that referenced this pull request Jan 14, 2014
@samccone
Merge pull request #866 from parklet/collectionview-show-events
94bd01b 
Trigger itemView.onShow in appendBuffer if collectionView is already shown
@samccone samccone merged commit 94bd01b into marionettejs:master Jan 14, 2014
@andrewhubbs andrewhubbs deleted the collectionview-show-events branch January 14, 2014 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CompositeView collection reset bug
2 participants
@andrewhubbs @samccone