Regions should better support animation

[paulfalgout]

https://github.com/marionettejs/backbone.marionette/pull/3156

Personally I'd like to see an implementation that does not introduce async library dependencies but where it easy to introduce your own.

[blikblum]

I think that we should look at prior work in order to accommodate most of use cases. Those are the solutions i found

v3 compatible:

• https://github.com/maratfakhreev/marionette-animated-region (uses velocity)
• http://codepen.io/blikblum/pen/oZxWjQ (uses CSS + matrix manipulation)

based on older Mn:

• https://github.com/jpdesigndev/MarionetteTransition/blob/master/src/scripts/MarionetteTransition.js (uses TweenLite)
• https://github.com/jmeas/marionette.transition-region/blob/master/example/script.js (animation type agnostic, examples use jQuery animation)
• https://github.com/marcinkrysiak1979/marionette.showAnimated (uses TweenMax)
• https://gist.github.com/jasonLaster/9794836 (uses CSS)

Supporting such animations https://tympanus.net/Development/PageTransitions/ is also desired

I will start to create demos with the various possibilities so we can design an abstraction

[JSteunou]

This one is quite old but might also help you https://github.com/JSteunou/marionette.csstransition-region ;)

[blikblum]

I just got working a proof of concept of animated Region using velocity, that can be see here: http://codepen.io/blikblum/pen/EWyaMv?editors=1010

Is still necessary to test the combination of options and create a version with CSS and jQuery animation but posting here initial notes to not forget

• Is necessary to override attachHtml and empty
• The overriden attachHtml is straight forward and is how is supposed to be used
• OTOH, in empty is necessary to copy Marionette implementation, still with some issues
  • If there's a hide animation, at the time _empty is called the currentView points to the new view, so the need to restore it since _empty deletes it
  • For the same reason above, when replaceElement is used, _restoreEl called inside _empty can be problematic.
• The options passed in show are not passed to attachHtml which could be useful to determine transition when calling show

[blikblum]

I created examples of how to implement animated Region with current version. Instead of overriding _empty, i override _removeView simplifying a bit:

• animate.css: http://codepen.io/blikblum/pen/PppGmG
• velocity: http://codepen.io/blikblum/pen/gmmwzg
• CSS Page transitions: http://codepen.io/blikblum/pen/peEqqM based on https://tympanus.net/Development/PageTransitions/
• jQuery fade: http://codepen.io/blikblum/pen/gmgWmK
• jQuery slide: http://codepen.io/blikblum/pen/VpPbRd

I think these examples covers most animation use cases

While not pretty and with some caveats (using replaceElement wont work) is relatively simple to add an animation:

class AnimatedRegion extends Marionette.Region {

  attachHtml(view) {    
    view.$el
      .css({display: 'none'})
      .appendTo(this.$el);      
    if (!this.removingView) view.$el.fadeIn('slow')
  }

  _removeView(view, shouldDestroy) {
    this.removingView = true;
    view.$el.fadeOut('slow', () => {
        super._removeView(view, shouldDestroy)
        this.removingView = false;
        this.currentView.$el.fadeIn('slow')
    })    
  }  
}

The attachHtml override is straight forward and i think that cannot get simpler than that.

OTOH, the _removeView override has the drawbacks of using an internal method and of needing to call the super method to handle shouldDestroy.

By promoting to an public method as removeView and refactoring to remove shouldDestroy handling we could write as:

class AnimatedRegion extends Marionette.Region {

  attachHtml(view) {    
    view.$el
      .css({display: 'none'})
      .appendTo(this.$el);      
    if (!this.removingView) view.$el.fadeIn('slow')
  }

  removeView(view) {
    this.removingView = true;
    view.$el.fadeOut('slow', () => {
        this.destroyView(view) // or view.remove if sure that view is a Mn.View
        this.removingView = false;
        this.currentView.$el.fadeIn('slow')
    })    
  }  
}

There's still the need of removingView flag since there are no way to know if there was previous view in attachHtml. By implementing a swappingView flag when a view is being replaced we could control if animate both in attachHtml and removeView. We could write the following:

class AnimatedRegion extends Marionette.Region {

  attachHtml(view) {    
    view.$el
      .css({display: 'none'})
      .appendTo(this.$el);      
    if (!this.swappingView) view.$el.fadeIn('slow')
  }

  removeView(view) {
    view.$el.fadeOut('slow', () => {
        this.destroyView(view) // or view.remove if sure that view is a Mn.View
        this.currentView.$el.fadeIn('slow')
    })    
  }  
}

I think is simple an easy to reason about, without the need to add more abstractions

One issue is that empty event would be fired before the element is effectively removed. But this is a side effect of async nature of animations and, if there's need to know when element is removed, simply triggering an custom event after this.destroyView(view) would handle it

I believe i can implement such changes easily.

[paulfalgout]

I'm not entirely sure how we'd refactor removeView without the shouldDestroy flag. This function is just there to take some of the logic out of _empty and it's main purpose is to decide whether to detach or destroy the view. If we made the function public though we could change it to { shouldDestroy: true } which could generally be ignored when overriding if the user is aware, but the argument would be self-explanatory to what it's for vs just a true/false arg.

It's kind of a bummer that life-cycle events wouldn't follow what's happening.. But in order to do that we'd have to add places where the events happen async.

I'm ok with keeping it simple, but then it's mostly recipes and developers handling and understanding the edge-cases, vs a feature which would have to account for the edge cases.

[paulfalgout]

What we could do is make complementary functions for overriding.

addView(view, options = {}) {
  if (options.shouldReplaceEl) {
    this._replaceEl(view);
    return;
  }

  this.attachHtml(view);
},
removeView(view, options = {}) {
  if (!options.shouldDestroy) {
    this._detachView(view);
    return;
  }

  if (view.destroy) {
    view.destroy();
  } else {
    destroyBackboneView(view);
  }
}

difficult thing would be that either replaceEl or detaching a view would mean calling a private function.. if we have to make those functions public as well (a difficult thing for _detachView) then this gets more complicated :-/

[blikblum]

I'm not entirely sure how we'd refactor removeView without the shouldDestroy flag.

I think it can be done with small changes. I will give a try so we can see how it goes

difficult thing would be that either replaceEl

Animating with replaceEl option is not practical since most of animation requires having both elements attached in the same time, something not related to Marionette at all. It could be possible with some bookkeeping to delay the replace at DOM level but, at first glance, would mess with the surrounding code that expects the new element after calling _replaceEl

So IMO we should avoid dealing with replaceEl code path when overriding for animation purposes.

[paulfalgout]

Animating with replaceEl option is not practical since most of animation requires having both elements attached in the same time

All the reason more to override that functionality.

If all you do is override attachHtml but you pass replaceElement things will break.

[blikblum]

If all you do is override attachHtml but you pass replaceElement things will break.

If replaceElement is passed attachHtml is not called at all.

Anyway, now using replaceElement is not compatible with animation.

The proposed solution will not change this fact even because there's no change in the order that the methods are executed.

A more comprehensive solution could be done in V4, changing the lifecycle to handle all cases.

[paulfalgout]

That's true that attachHtml would not be called, but restoreEl followed by the overridden removeView would be called when emptying. Probably ok.. but the view in removeView would be detached.

What would a more comprehensive solution look like? I guess I'm not certain why it would be a breaking one.

[blikblum]

restoreEl followed by the overridden removeView would be called when emptying. Probably ok.. but the view in removeView would be detached.

The animation would not work although i think there will be no error since the view is already being destroyed while detached when replaceElement is used

To get animation work with replaceElement it should be necessary to override replaceEl with potential to complicate things. IMO forbidding replaceElement in a region that is animated is a fair tradeoff. Anyway, i will look how it could animate also with replaceElement

[paulfalgout]

I don't know that I'm concerned with animation working with replaceElement TBH. But if you have a region where you've overridden removeView for any purpose (once it's public it's public 😭 ), maybe not even animation, and you do use replaceElement the view will no longer be in the DOM.. but only in that one edge-case would that be true.. depending on what you're doing in removeView if you're expecting the view to be in the DOM, you might have an issue.

actually I think we have a related bug currently in v3.2. restoreEl should be marking the view as not attached and should trigger detach events right? I'm pretty sure the view is getting the events now when it is destroyed, but they're incorrect.. the view would already be detached.

if we fix that at least view.isAttached() would work within removeView

[blikblum]

I don't know that I'm concerned with animation working with replaceElement TBH

I tried to implement it and gave up. Its something hard even manipulating DOM with vanilla js. And to get it working the implementation would be difficult to reason about.

But if you have a region where you've overridden removeView for any purpose (once it's public it's public 😭 ), maybe not even animation, and you do use replaceElement the view will no longer be in the DOM.

True. For the default implementation should be no problem as this is already occurring. The issue is the expectation / code contract. I see two possibilities: document that with replaceElement is not guaranteed that the view will be attached or change the method name to avoid the expectation, hinted by the name, that the view is in the DOM