Make a public detachView

[paulfalgout]

This function removes the need for a preventDestroy option. #2640

Essentially if you want to preventDestroy just detach the view first. When you want to preventDestroy you already have to have a copy of the view.. and depending on what is doing the view emptying preventDestroy can become a round about API.

What you really want is

const myView = myRegion.currentView;
myRegion.detachView();
myOtherRegion.show(myView);

Upon approval:

• Tests
• Docs

[rafde]

how about

var view = parentView.detachChildView('region');
myOtherRegion.show(myView);

which proxies detachView. That way people don't directly interact with the region.

[coveralls]

Coverage decreased (-0.4%) to 99.604% when pulling ba4c9d7 on paulfalgout:region-detachview into 8d41fc5 on marionettejs:next.

[paulfalgout]

I think a detachChildView makes since, but I'd think it'd be added along side.. similarly to showChildView where that's the suggested API, but you can interact with the Region if need be. And I think #3156 is a reason you may want to..

[JSteunou]

Hooo I like those ideas. @rafde your idea of an API from layout point of view is cool!

[rafde]

Yeah, I'm 👍 for this. Someone in gitter wanted something like this. @paulfalgout mind if I help with the View#detachChildView part? Or just general help? Or you got this?

[rafde]

detachChildView(region){
  return this.getRegion(region).detachView();
}

detachView() {
  const view = this.currentView;
  if(!view){
    return;
  }
  delete this.currentView;
  this._detachView(view);
  delete view._parent;
  return view; // I think this should return the view getting detached
},

[paulfalgout]

@rafde you can take it over if you want.

good call about the return if !view
And you're totally right about returning the view.. I almost did that at first, but every other function was returning this.. however add/remove functions elsewhere return the view.

[rafde]

Ah right, @paulfalgout. I get that you don't want to write up the test, 😄 .
Can this work be placed in the marionettejs repo branch? Or should I just PR against your fork branch?

[paulfalgout]

@rafde we can move it, or you can PR on my branch, or just close this and reopen another.. I'm up for whatever.. I mean I'll also do it eventually, but if you wanted to step in 👯

[rafde]

I'll PR against yours for now.
I'm thinking we should start creating repo branches for collab work.

[rafde]

@paulfalgout paulfalgout#4 forgot the docs, though.

[coveralls]

Coverage remained the same at 100.0% when pulling 3f4e701 on paulfalgout:region-detachview into 8d41fc5 on marionettejs:next.

[ahumphreys87]

Nice i like this. Should we add a deprectation notice to preventDestroy?

[rafde]

forgot to do a region exist check

  const region = this.getRegion(name);
  if (!region) {
    return;
  }
  return region.detachView(); 

[paulfalgout]

FWIW that check isn't being done in other places

[rafde]

welp, I am done here. 👍

[JSteunou]

Nice! Just concerned about that check on getRegion(''). I understand adding the check is not a thing in Marionette src code and it would silent catch an error making difficult to debug, but could checking + logging in debug mode be a nice feature?

[rafde]

@JSteunou I am not sure what you are looking for. Snippet of what you mean by "checking + logging in debug mode", please.

[paulfalgout]

I'm in general more interested in consistency over completeness. Even if it is decided that we should be throwing a warning in debug or an error or whatever.. we should handle that in a later issue and across the lib.

[rafde]

I am for that sentiment.

[paulfalgout]

@ahumphreys87 #3168

[JSteunou]

@paulfalgout completely agreed, just throwing the ball. Wouldnt make this PR fuzzy with a lot of changes not related.

Nice work btw guys 👍

[scott-w]

I'll add a 👍 too