add a call when the view has been swapped #11

Closed
wants to merge 1 commit into
from

4 participants

@samccone

when a view has been swapped into the dom I wanted to be able to be notified on the view's instance

this allows for that functionality

@prabirshrestha

was about to write a PR and saw this PR.

+1

needed this feature to set focus on the text input after it has been attached to the dom.

@jferris jferris commented on the diff Jan 31, 2013
...ssets/javascripts/backbone-support/swapping_router.js
@@ -10,6 +10,8 @@ _.extend(Support.SwappingRouter.prototype, Backbone.Router.prototype, {
this.currentView = newView;
$(this.el).empty().append(this.currentView.render().el);
+
+ typeof this.currentView.swapped == "function" && this.currentView.swapped();
@jferris
thoughtbot, inc. member
jferris added a line comment Jan 31, 2013

A few notes:

  • I'd rather see an explicit if than rely on the short-circuit nature of compound conditions.
  • currentView needs all the methods that CompositeView has, so you could add this as a no-op function to that prototype and drop the condition entirely.
  • We've been using events more than template methods, so an event would fit in better with the rest of the library.
  • We already have a leave event. How is that different from swapped?
@prabirshrestha
prabirshrestha added a line comment Jan 31, 2013

We've been using events more than template methods, so an event would fit in better with the rest of the library.

Event would be good too. But it should aslo be triggered in the view and not just the router.

We already have a leave event. How is that different from swapped?

I want to be notified when the view has been appended to the dom.

@jferris
thoughtbot, inc. member
jferris added a line comment Jan 31, 2013

Okay, that makes sense. So, here's what I think should change:

  • Add a swapped method to CompositeView.
  • Call it unconditionally from SwappingRouter after swapping in the new view.
  • Trigger a swapped event from CompositeView when swapped is called.

Sound good?

@jferris
thoughtbot, inc. member
jferris added a line comment Jan 31, 2013

Oh - I forgot to mention - this project is 100% TDD, so we aren't merging in any pull requests without tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jferris
thoughtbot, inc. member

Sorry for the delay on this. I have some concerns that I outlined in an inline comment.

@christoomey christoomey added a commit that referenced this pull request Apr 27, 2013
@christoomey christoomey Add 'swapped' method / event to composite_view
Allows for triggering events when the new view is added to the DOM

- SwappingRouter calls the swapped method if it exists
- CompositeView emits 'swapped' event

Closes pull request #11 - 'Add a call when the view has been swapped'
9558f0a
@christoomey christoomey added a commit that referenced this pull request Apr 27, 2013
@christoomey christoomey Add 'swapped' method / event to composite_view
Allows for triggering events when the new view is added to the DOM

- SwappingRouter calls the swapped method if it exists
- CompositeView emits 'swapped' event

Closes pull request #11 - 'Add a call when the view has been swapped'
5a1debe
@christoomey

We have merged into master a slightly modified version of this functionality in 5a1debe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment