airbnb / infinity

UITableViews for the web (DEPRECATED)
http://airbnb.io/infinity/
Other
2.8k stars 281 forks source link

this.$el.remove() unbinds all jQuery data and event handlers #13

Closed banks closed 11 years ago

banks commented 12 years ago

Currently when a Page is stashed to save memory the visible DOM is removed with this.$el.remove() in Page.stash(). This causes all jQuery data and event handlers to be removed from the ListItems removed.

When you scroll back up and those elements are restored, they no longer respond to events or contain data bound prior to the stashing.

I can trivially fix this by replacing replace() calls with detach() calls in each of stash(), appendTo() and prependTo() methods of Page.

I think this should be all that is required. Page.remove() and ListItem.remove() should keep jQuery remove() semantics and that should be sufficient to ensure that old stuff is cleaned up correctly when actually removed whilst maintaining bound data and events through stashing.

Is that reasoning about where to keep and where to remove data and events sound? I can submit a pull request for this if so.

reissbaker commented 12 years ago

Makes sense. Realistically, I'd caution against putting event handlers or data on the elements themselves -- in an infinite list, that's going to eat a lot of memory. That said, destroying the handlers and data is pretty unexpected/bad behavior; I'll slot it for the next sub-version release. Feel free to submit the pull request!

aredridel commented 12 years ago

Indeed -- jquery 1.7 style event handlers, bound to a containing element but restricted by a selector are the way to go.

reissbaker commented 11 years ago

Merged in @saiwong's changes, which should fix this.

banks commented 11 years ago

Thanks @saiwong, @reissbaker.