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

Deprecate CollectionView Empty View #2495

Closed
jasonLaster opened this issue Apr 14, 2015 · 7 comments
Closed

Deprecate CollectionView Empty View #2495

jasonLaster opened this issue Apr 14, 2015 · 7 comments
Milestone

Comments

@jasonLaster
Copy link
Member

I'd like to suggest that we deprecate the empty view functionality in collection and composite view.


The rationale is that the logic for when to show an "empty" view would better be handled by a containing view that either shows a list or an empty view. Further more, when, where and how to show empty views is up to interpretation.

Also, to be honest, as a maintainer, CollectionView is one of the most complicated classes in the library and I believe it would be significantly easier to maintain w/o supporting empty views


The road forward, I would like to land in a place that is better for everyone. We should create a small library or even a test case that demonstrates how we envision creating a layout view with a list and empty view region.


So, this sounds great, but isn't it really hard / risky to remove the empty view code... Good question, I would think so, but I just did it i think.

https://gist.github.com/jasonLaster/52a36178a996ad03e6a5


Next steps, if we agree on this path forward somebody should grab the patch and make it happen

@jasonLaster jasonLaster added this to the v3.0.0 milestone Apr 14, 2015
@jasonLaster
Copy link
Member Author

We'd probaby want to create a small example repo or test for the shim

AbstractView.extend({
  constructor: function() {
   this.listenTo(this.collection, 'onChange')
  },
  onChange: function() {
    if(this.wasSome && this.collection.length == 0) {this.triggerMethod('collection:empty')}
    if (this.wasEmpty && this.collection.length > 0) {this.triggerMethod('collection:some')}
  },
}

View.extend({
  onCollectionEmpty: function() {
    this.showView('body', new EmptyBodyView());
  },

  onCollectionSome: function() {
    this.show('body', new ListView())
})

or add some hooks so it's easy to shim

var MyCV = Marionette.CollectionView.extend({
  onEmpty: function () {
    this.show(new MyEmptyView());
    // or
    this.showEmptyView(new EmptyView());
  }
});

@ianmstew
Copy link
Member

Thanks for starting this discussion, @jasonLaster! I agree that it's actually not intuitive for a CollectionView to show an edge case alternative to a "child view". For example, a pure array doesn't have an "empty value".

On a purely logical level, swapping out the entire CollectionView for an alternative view at the parent Region makes the most sense, at the cost of upgrade burden. On the flip side, providing hooks within the CollectionView makes the most sense from an "ease of upgrading" perspective, at the cost of logical purity and some extra core code (although not nearly as much as supporting the EmptyView we have today).

If we choose to go with the first, I recommend including an "empty view" example in our first class CollectionView documentation, or at the least a link to an example. I.e., give some serious hand holding to educate on "polymorphic" region usage.

@jasonLaster
Copy link
Member Author

proposal: child view kills all the empty view code and fires an empty and a some event, which the parent listens to and reacts

View.extend({
  childEvents: {
    'empty': 'onCollectionEmpty',
    'some': 'onCollectionSome'
  },

  onCollectionEmpty: function() {
    this.showView('body', new EmptyBodyView());
  },

  onCollectionSome: function() {
    this.show('body', new ListView())
})

@JSteunou
Copy link
Contributor

Could it be a behavior?

Marionette could start to pack in some behaviors and document and to use EmptyView behavior with collection / composite view.

@jasonLaster
Copy link
Member Author

yea, id be down with that

@ianmstew
Copy link
Member

@jasonLaster, I just thought of a potential complexity with the proposal that I want to iron out.

When it comes to the CollectionView filter, the CollectionView defines the filter and applies it after changes to the collection. If we are replacing the CollectionView with an EmptyView using Region#show's default destroy behavior, then we lose the special filter listener when the CollectionView destructs.

For an EmptyView Behavior to work, it would have to cause the EmptyView swap out to be non-destructive to the CollectionView to keep collection listeners alive, and then swap back in the same CollectionView instance onCollectionSome. The caveat here is that if the EmptyView is replaced for any other reason, then both EmptyView and the CollectionView will then need to be destroyed together.

@cmaher, you're the filter guy, thoughts?

@ahumphreys87
Copy link
Member

gonna close this and cover this as part of #2560

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

No branches or pull requests

4 participants