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

Region does not re-delegateEvent on previously closed view #654

Closed
wants to merge 1 commit into from
Closed

Region does not re-delegateEvent on previously closed view #654

wants to merge 1 commit into from

Conversation

diwu1989
Copy link

This is a fix for #651

Region.show will call delegateEvents on the view if the view is one that was previously closed.

Test included.

@diwu1989
Copy link
Author

Any comments? Would appreciate this getting merged in and released, I'd like to be running off of mainline marionette instead of my monkey-patched marionette. Thanks.

@jabberfest
Copy link

Looks like a dup. of this except you made a pull request :-) I hope it gets accepted. +1 for this

#622

@diwu1989
Copy link
Author

Yeah, I think this definitely should go in, or else the documentation for Region should be updated to reflect that the UI, event, and model bindings will need to re-delegated manually.

@firetix
Copy link

firetix commented Jun 29, 2013

I was struggling with this for the past few days, this is awesome really need to be integrated

@andrewhubbs
Copy link
Contributor

👍 Been dealing with this same issue. Would love to see this merged in.

@diwu1989
Copy link
Author

diwu1989 commented Jul 3, 2013

If anyone is feeling adventurous:
https://raw.github.com/diwu1989/backbone.marionette/rebuildWithRegionFix/lib/backbone.marionette.js

I rebased this pull request off of the current head of Dev, and then rebuilt with the fix into that other branch.

@nrako
Copy link

nrako commented Aug 13, 2013

Is this PR the right way to fix this @derickbailey ?

@jabberfest
Copy link

Looks like he fixed this in 1.1 with his own fix? This is a quote from the v1.10 change log released a few days ago.

v1.1 view commit logs

Marionette.View / All Views

Fix for ui bindings to not be removed from view prototype, if unrendered view is closed

@diwu1989
Copy link
Author

I don't think that commit log message is the same problem. Specifically, try this jsfiddle:
http://jsfiddle.net/X9PE6/6/

It's pointing towards the latest 1.1 build and it still doesn't work.

@farias-r7
Copy link

Yea you're right. I didn't test what I was thinking when I wrote that. Maybe we can get some feedback from @derickbailey

@paynecodes
Copy link

👍 for this. I'm having trouble with ui hash events upon closing and reopening the view. For now, I'm adding the selector onRender like so:

onRender: function() {
  this.$shouldBeUI = $('#some-id');
}

This is what I was using before realizing that this.ui.shouldBeUI wasn't available after navigating to and fro and bit.

ui: {
  'shouldBeUI': '#some-id'
}

@@ -132,7 +132,12 @@ _.extend(Marionette.Region.prototype, Backbone.Events, {
if (isDifferentView || isViewClosed) {
this.open(view);
}


if (isViewClosed) {
Copy link
Member

Choose a reason for hiding this comment

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

only commit this change, not all the whitespace changes

@samccone
Copy link
Member

👍 once the whitespace stuff is fixed

@ccamarat
Copy link
Contributor

@jpdesigndev I published a fix for the UI issue about a month ago; is it in your codebase, and are you still seeing that problem? #686

@samccone
Copy link
Member

@diwu1989 is this still active?

@cobbweb
Copy link
Member

cobbweb commented Sep 25, 2013

I think the semantics of this are wrong. Isn't close suppose to be a destructive method to ready the view for garbage collection?? If #724 make it in, this would be more obvious.

A better fix here would be a new method on Marionette.ItemView that 'closes' a view in a way that allows it to be shown/opened again (and therefore not ready for garbage collection). Or just re-make the view and show that instead.

@samccone
Copy link
Member

yeah @cobbweb I agree with you it seems like several of these PR's are really just trying to fix what #724 does.

I was just checking to see if @diwu1989 had any comments or interest in the PR.

@diwu1989
Copy link
Author

If the other PR fixes this issue, then please just go ahead with that one.

Thanks!

@diwu1989 diwu1989 closed this Sep 25, 2013
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

Successfully merging this pull request may close these issues.

None yet

10 participants