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.

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

Remove mvc/View#mapChildViewEvent #123

Open
zship opened this issue Jul 2, 2014 · 6 comments
Open

Remove mvc/View#mapChildViewEvent #123

zship opened this issue Jul 2, 2014 · 6 comments
Milestone

Comments

@zship
Copy link
Contributor

zship commented Jul 2, 2014

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.

@zship zship added this to the 3.0 milestone Jul 2, 2014
@kellyrmilligan
Copy link
Contributor

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
Copy link
Contributor Author

zship commented Jul 3, 2014

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
Copy link
Contributor

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
Copy link
Contributor Author

zship commented Jul 3, 2014

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
Copy link
Contributor

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
#123 (comment).

@zship
Copy link
Contributor Author

zship commented Jul 3, 2014

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

@zship zship closed this as completed Jul 3, 2014
@zship zship reopened this Jul 3, 2014
@zship zship removed the enhancement label Jul 21, 2014
@zship zship removed the v3 label Jul 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants