mutualmobile / lavaca

A modern framework for client-side MVC web applications
Other
105 stars 23 forks source link

Remove mvc/View#mapChildViewEvent #123

Open zship opened 10 years ago

zship commented 10 years ago

I'd like to know if anyone knows what this is for. Doing a git blame shows that Kelly added it under 7c64abd "refactoring view classes..." Of the projects [PropertyCross, autotrader-web, kbb2, kbb, lavaca-starter, nike-video, selectcomfortproto], it's only used in one view in kbb2. If I'm understanding right, I think that regular old DOM events (mapped with mapEvent in the parent) would be just as effective here.

kellyrmilligan commented 10 years ago

Well it in theory it was for communicating between parent and child views so eh parent view could listen for child events. Seemed to make sense at the time :). A central event dispatcher could also do the trick

Sent from my iPhone

On Jul 2, 2014, at 2:49 PM, Zach Shipley notifications@github.com wrote:

I'd like to know if anyone knows what this is for. Doing a git blame shows that Kelly added it under 7c64abd "refactoring view classes..." Of the projects [PropertyCross, autotrader-web, kbb2, kbb, lavaca-starter, nike-video, selectcomfortproto], it's only used in one view in kbb2. If I'm understanding right, I think that regular old DOM events (mapped with mapEvent in the parent) would be just as effective here.

— Reply to this email directly or view it on GitHub.

zship commented 10 years ago

Hey, Kelly! Didn't expect to see you here. Thanks for chiming in! Honestly I've only ever done DOM event handling with jQuery, but I was thinking: wouldn't doing something like $.trigger from the child be equivalent?

georgehenderson commented 10 years ago

The idea was to use the built in event dispatcher apart of the prototype of View to communicate up the chain. The problem is the events don't bubble past the parent view by default. This whole issue is avoided in practice if your okay with a childview knowing about its parent. this.parentView.trigger('yoDoSomething') Gross I know, but effective.

zship commented 10 years ago

And now George! So we were reusing the EventDispatcher part. That does make some sense, though I wonder why we didn't just use DOM events instead of EventDispatcher for Views in the first place. I vaguely remember reading something about performance and # of DOM events a while back, but the only slowdown I've ever seen had to do with large numbers of handlers actively being called.

kellyrmilligan commented 10 years ago

yeah, i'm fine without it as well. it was modeled after how model events are bubbled up in a collection. but maybe this is overkill.

On Thu, Jul 3, 2014 at 9:38 AM, Zach Shipley notifications@github.com wrote:

And now George! So we were reusing the EventDispatcher part. That does make some sense, though I wonder why we didn't just use DOM events instead of EventDispatcher for Views in the first place. I vaguely remember reading something about performance and # of DOM events a while back, but the only slowdown I've ever seen had to do with large numbers of handlers actively being called.

— Reply to this email directly or view it on GitHub https://github.com/mutualmobile/lavaca/issues/123#issuecomment-47937854.

zship commented 10 years ago

Hmm... would there be a down-side to y'all? My line of thinking is:

  1. Views basically represent one DOM node so it's a natural fit
  2. Less work and our code will be less complex
  3. Possibly lower cognitive burden on users