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

addChildViewEventForwarding available from ItemView #790

Closed
wants to merge 1 commit into from
Closed

addChildViewEventForwarding available from ItemView #790

wants to merge 1 commit into from

Conversation

lukesargeant
Copy link
Contributor

When developing with Marionette, child item views forward their
events to their parent composite/collection view.

Sometimes there is the need to manually create a subview in an ItemView
or Layout and in this situation it would be great to be able to use
event forwarding in the same way.

This pull request allows addChildViewEventForwarding to be used by
custom code in ItemViews or Layouts to setup the same functionality
provided by default in collection/composite views.

For example:

var ItemViewWithForwarding = Marionette.ItemView.extend({
    ...
    renderChildView: function(category) {
        var view = new FormCategoryListItemView({
            model: new Backbone.Model({category: category})
        });

        //Provide view reference and event prefix
        this.addChildViewEventForwarding(view, "childview");
        this.$('.event-actions').append(view.render().el);
    }
});

Would love to hear feedback on this.

When developing with Marionette child item views forwarding their
events to their parent composite/collection view is very useful.

Sometimes there is the need to manually create a subview in an ItemView
or Layout and in this situation it would be great to be able to use
event forwarding in the same way.

This pull request allows addChildViewEventForwarding to be used by
custom code in ItemViews or Layouts to setup the same functionality
provided by default in collection/composite views.
@jamesplease
Copy link
Member

Interesting thoughts – I'm doing something similar with events over Wreqr channels. We should revisit this for v2+

@jamesplease
Copy link
Member

This PR is one of four that are > 3 months old. I set it as critical so that we expedite resolving it.

@@ -382,4 +382,48 @@ describe("item view", function(){
});
});

describe("when a view set as a child with addChildViewEventForwarding triggers an event", function() {
Copy link
Member

Choose a reason for hiding this comment

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

I think this test should be moved to view.spec.js if it's a general view feature.

@jasonLaster
Copy link
Member

Sorry @lukesargeant for the slow reply. Overall, this looks good. I can see this being useful for layouts.

@jasonLaster
Copy link
Member

also, i wonder if you can rename addChildViewEventForwarding to forwardEvents

@samccone
Copy link
Member

So this raises a valid issue, it is HARD to listen and unbind from a childView rendered into a region correctly. You end up with something looking like this
this.listenTo(this.contentRegion.currentView, "bam", doSomething)

Which obviously is gross.

In my mind the ideal this would be for the region to receive these events and then as someone who wants to listen to a childView rendered into a region do something like this.

Marionette.Layout.extend({
  regionEvents: {
    "bam": "doSomething"
  }
})

So now we become inline with collectionEvents, modelEvents, events.
This also becomes very declarative and the end user is no longer concerned with the current "State" of the region(s).

So I think this PR should be based on the discussions here.
#668

@jasonLaster
Copy link
Member

@samccone I like regionEvents a lot. I've run into this exact thing several times.

I think I'd prefer to focus on having region events propogate to the layout than solve the problem for all child views. I say this because regions are the encouraged way to have child views and based on what we find, we could loosen the api down the road to enclude a generic event fowarding library.

@samccone
Copy link
Member

I agree. ok this is great!

@paulovieira
Copy link
Contributor

Nice! In the last days I've also been thinking about implementing this feature. I guess this is definitely something in demand.

Some thoughts about it:

the hierarchy of inclusions is

  1. the parent view myParent is at the top (a Layout instance)
  2. myParent has a region myRegion
  3. myRegion has a subview mySubview

So it seems natural to me that the events triggered by mySubview should be automatically forwarded to its myRegion. The parent layout myParent should then do something to listen to these events. For consistency the forwarding of the events should be similar to what is done in CollectionView

myParent = Marionette.Layout.extend({

    regions: {
        "#my-region": myRegion
    },

    initialize: function(){
        // listeningTo, or better yet, introduce a regionEvents
        this.listenTo(this.myRegion, "subview:foo", function(){ ... } );
    }

});


// the subview could be anything: itemview, collectionview, layout, etc
mySubview = Marionette.ItemView.extend({

    // similar to itemViewEventPrefix in CollectionView
    subviewEventPrefix: "subview",

    // the "foo" event will automatically be forwarded to the region that contains it as "subview:foo" 
    triggers: {
        "click": "foo"
    }
})

However this brings a small issue: when we invoke

parentView.myRegion.show(mySubview) 

we currently have the following events:

  1. "show" - triggered on the region instance when the view has been rendered and displayed.
  2. "show" - triggered on the view instance when the view has been rendered and displayed.

With the above approach, the event described in 2) would reflect back to the region as "subview:show". This is redundant because it was the region itself who took the action to trigger that event :-)

It's odd behaviour, but I don't see any harm in this. It would be consistent at least.

@lukesargeant
Copy link
Contributor Author

Wow looks like I missed the discussion on this one!

My main use case for this was creating a parent view in a controller and wanting to listen to its child views events through it - the same way you can with composite views. Therefore I was looking for more of a regionTriggers rather than regionEvents solution.

@samccone
Copy link
Member

sure @lukesargeant this is doable with regionEvents and will remove a single level of event passing

something like this

regionEvents: {
  foo: function(){this.trigger.apply(this, arguments)}
}

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

Successfully merging this pull request may close these issues.

5 participants