Simplify API for child views #238

pepperbc · 2016-04-04T19:46:38Z

When creating child views, it is a common pattern to initialize the child view in one place, and then inject it in "attachTrackedViews". This could be simplified by always injecting child views unless they have been hidden/turned off/etc. If the child view to be attached a certain injection point is to be redefined, it would only have to be redefined once, and then would always render with the new child. This would reduce boilerplate injections in the render() method, and also reduce boilerplate hide/show methods on Views.

Potential API:

intialize: function() {
this.attachChildView('my-child-view', new ChildView());
}

hideChild: function() {
this.hideChildView('my-child-view');
}

showChild: function() {
this.showChildView('my-child-view');
}

setChild: function(childView) {
this.attachChildView('my-child-view', childView);
}

mandragorn · 2016-04-05T01:59:39Z

@pepperbc can you please give an example of a complex view with the current API and how it would change with this API. I would like to see a side-by-side comparison of the APIs so that I can better see what this is trying to address.

pepperbc · 2016-04-06T21:04:01Z

TorsoView.extend({
  events : {
    'click.openCollapsibleSection1' : 'openCollapsibleSection1',
    'click.openCollapsibleSection2' : 'openCollapsibleSection2'
  },
  initialize: function() {
    this._collapsibleSection1 = new CollapsibleSection();
    this._collapsibleSection2 = new CollapsibleSection();
    this._asideView = new AsideView();
    this._footerView = new FooterView();

    this.set('showCollapsibleSection1', false);
    this.set('showCollapsibleSection2', false);

    this.listenTo(this.viewState, 'change:showCollapsibleSection1', this.render);
    this.listenTo(this.viewState, 'change:showCollapsibleSection2', this.render);

    this.listenTo(this._collapsibleSection1, 'CLOSE', function() {
      this.set('showCollapsibleSection1', false);
    });
    this.listenTo(this._collapsibleSection2, 'CLOSE', function() {
      this.set('showCollapsibleSection2', false);
    });
  },
  openCollapsibleSection1: function() {
    this.set('showCollapsibleSection1', true);
  },
  openCollapsibleSection2: function() {
    this.set('showCollapsibleSection2', true);
  },
  attachTrackedViews: function() {
    var showCollapsibleSection1 = this.get('showCollapsibleSection1');
    var showCollapsibleSection2 = this.get('showCollapsibleSection2');

    if (showCollapsibleSection1) {
      this.attachView('collapsible-section-1', this._collapsibleSection1);
    }
    if (showCollapsibleSection2) {
      this.attachView('collapsible-section-2', this._collapsibleSection2);
    }

    this.attachView('aside', this._asideView, { shared: true });
    this.attachView('footer', this._footerView, { shared: true });
  },
  changeAside: function(newAsideView) {
    this._asideView = newAsideView;
    this.render();
  }
});


TorsoView.extend({
  events : {
    'click.openCollapsibleSection1' : 'openCollapsibleSection1',
    'click.openCollapsibleSection2' : 'openCollapsibleSection2'
  },
  initialize: function() {
    this.setChildView('collapsible-section-1', new CollapsibleSection());
    this.setChildView('collapsible-section-2', new CollapsibleSection());
    this.setChildView('aside', new AsideView());
    this.setChildView('footer', new FooterView());

    this.listenTo(this.getChildView('collapsible-section-1'), 'CLOSE', function() {
      this.hideChildView('collapsible-section-1');
    });
    this.listenTo(this.getChildView('collapsible-section-2'), 'CLOSE', function() {
      this.hideChildView('collapsible-section-2');
    });
  },
  openCollapsibleSection1: function() {
    this.showChildView('collapsible-section-1');
  },
  openCollapsibleSection2: function() {
    this.showChildView('collapsible-section-2');
  },
  changeAside: function(newAsideView) {
    this.setChildView('aside', newAsideView);
  }
});

pepperbc · 2016-04-06T21:05:35Z

Basically it removes the need for redundant injections in "attachTrackedViews", and redundant render listeners when you hide/show/first inject a child view. The additional show/hide functionality could potentially remove some boilerplate functions depending on the use case. I'm sure there are some edge cases I'm forgetting about - any ideas what they are?

kentmw · 2016-04-06T21:22:32Z

This makes a few assumptions. Just pointing them out so we know what comes with this.

First, that you use the same injection site name always (even so far as changing the template you are using if someone overrides the updateDOM method).

Second, the check to find out if you should inject/not inject a view is able to be stored prior to render (meaning you have no inspection of transient state, like current dom, time, variables in other services/models at time of render etc.)

Third, I assume you would have a shared tracked view version - you dropped the aside and footer shared part in your simplification.

Fourth, you're combining the registering child view with the attaching it to the DOM. This probably was an easy step for you because in attachView we already register the child view. But, it's important to realize and make obvious that a child view can be registered and bound to a parent without having to tie it to the parent's DOM.

There's a big shift here from making rendering choices inside the render process, and making them before the rendering process. I think you, Ben, like the idea of making some of those choices decoupled from the rendering process, because it feels better to be making what seem like more semantic choices in the methods that pertain to hiding/showing method rather than to set variables and wait until the next render. I also think this is a useful addition.

What I fear is this is a simplification of the process (based on the above mentioned assumptions). It would be nice if we could add additions that simplify common processes that don't restrict complicated cases and if possible highlight the process that happens under the hood, not hide it.

All that said, I'd like to see if we can play with your idea to get show/hide to work while maintaining the ability to control the rendering logic if need be.

kentmw · 2016-04-06T22:27:32Z

From my conversation with @pepperbc. These are the problems/issues that are causing a need for change:

attachTrackedViews is unclear when this it runs and this can cause confusion about what to put it in. Is it run once on initialize or every render?
It would be nice to have a closer control of the attaching/detaching views in methods other than attachTrackedViews. How can we give this control while still holding true and obvious about the rendering process?
We are orphaning child views that are initialized in the initialize method but never attached. This could be a big problem if it has _activate logic and is activated manually.

mandragorn · 2016-04-07T13:25:42Z

I like the idea of helper methods that augment instead of replace the existing functionality. Personally I'd like to keep the attach view logic in the same place, but add helpers for show/hide. The main benefit I see is adding a streamlined mechanism for show/hide that bundles the listeners and view state property setup. But even as I'm saying this I realize it all makes sense when you've worked with Torso, but makes what is happening under the hood opaque to people who are not familiar with it.

My first instinct is to add helper methods that do the same setup we do now:

TorsoView.extend({
  events : {
    'click.openCollapsibleSection1' : 'collapsible-section-1--show',
    'click.openCollapsibleSection2' : 'collapsible-section-2--show'
  },
  initialize: function() {
    this._addTrackedView({
      alias: 'collapsible-section-1', // allows us to edit the config later and change properties.
      view: new CollapsibleSection(),
      injectionSite: 'collapsible-section-1', // default to alias if not set?
      listenTo: {
        'CLOSE': 'hide'
      }
    });
    this._addTrackedView({
      alias: 'collapsible-section-2', // allows us to edit the config later and change properties.
      childView: new CollapsibleSection(),
      listenTo: {
        'CLOSE': 'hide'
      }
    });
    this._addTrackedView({
      alias: 'aside',
      view: asideView,
      sharedView: true
    });
    this._addTrackedView({
      alias: 'footer',
      view: footerView,
      sharedView: true
    });
  },
  changeAside: function(newAsideView) {
    this._updateTrackedView({
      alias: 'aside',
      view: newAsideView
    });
  }
});

I'm not sold on the api above, just thinking through an alternative (and this mirrors form and object model tracking so there is some consistency in API).

Configuration Options:

{
  alias: String // unique identifier for this view - used to identify anything specific to this view.
  view: TorsoView // the view instance (could do a constructor support if there is a use case)
  shared: Boolean // the view is a shared tracked view, not a child.
  listenTo: { // like events hash on view but for the parent view listening to the events on the child view.
    '<event name>': String or Function // String options: 'show', 'hide';
  },
  shouldRender: Function // this is run to determine if the view should be shown.  Will replace show/hide logic (so any show or hide methods don't work.  alternatively the show/hide value could be passed in as an argument to the function and it can process the value).
}

Automatically generated functions that you can use and reference:
--show - show view.
--hide - hide view.

mandragorn · 2016-04-07T13:27:30Z

the API above changed a lot while I was writing it and it lost some of my thoughts from the first paragraph... I'm not entirely sure I like the abstraction, but it does let us configure views in a more concise way.

We could also have shortcut or overloaded methods for common use cases (like the form model tracking API).

pepperbc · 2016-04-07T14:39:24Z

In terms of making what's happening under the hood opqaue, I feel like all of these (including the current implementation) are difficult to see the inner workings through - without knowing Torso pretty well, you're just going to be guessing anyhow. 99% of times we're going to be using the basic cases anyhow, so the edge cases just need to be possible and documented, but not necessarily as intuitive as the standard cases (though obviously both is preferred).

@mandragorn Are the changes in that API :

Addition of alias/injectionSite
listenTo (maybe change to events for consistency?)
Object arguments instead of two arguments and an options object

How do you feel about breaking your subview objects into an array? Then the initialize method could just be for actual initialization, and not configuration as well. I feel that would promote readability in a lot of classes. I made a couple of other minor API changes. There's still a few issues with this way, but I like the readability:

TorsoView.extend({
  events : {
    'click.openCollapsibleSection1' : function() {this.showChildView('collapsible-section-1')},
    'click.openCollapsibleSection2' : function() {this.showChildView('collapsible-section-2')}
  },
  children: [
    {
      alias: 'collapsible-section-1', // allows us to edit the config later and change properties.
      view: new CollapsibleSection(),
      injectionSite: 'collapsible-section-1', // default to alias if not set?
      events: {
        'CLOSE': 'hideSection1'
      }
    },
    {
      alias: 'collapsible-section-2', // allows us to edit the config later and change properties.
      childView: new CollapsibleSection(),
      listenTo: {
        'CLOSE': 'hideSection2'
      }
    },
    {
      alias: 'aside',
      view: function() {return this._asideView;},
      sharedView: true
    },
    {
      alias: 'footer',
      view: function() {return this._footerView;},
      sharedView: true
    }
  ],

  initialize: function() {
    this._asideView = new AsideView();
    this._footerView = new FooterView();
  },

  changeAside: function(newAsideView) {
    this._updateTrackedView('aside', {view: newAsideView});
  }
});

kentmw · 2016-04-07T14:46:21Z

(In the car on my phone)
Make the children field an object and the alias the key. Also you'd have to
allow the parent to set the child view in the initialize to grab run time
views (defining the child view on the class level is rarely going to be
useful especially for views that aren't singletons). And do we define all
possible child view mappings on the class level or just the ones we want
initially?

On Thu, Apr 7, 2016, 10:39 AM pepperbc [email protected] wrote:

In terms of making what's happening under the hood opqaue, I feel like all
of these (including the current implementation) are difficult to see the
inner workings through - without knowing Torso pretty well, you're just
going to be guessing anyhow. 99% of times we're going to be using the basic
cases anyhow, so the edge cases just need to be possible and documented,
but not necessarily as intuitive as the standard cases (though obviously
both is preferred).

@mandragorn https://github.com/mandragorn Are the changes in that API :

Addition of alias/injectionSite

listenTo (maybe change to events for consistency?)

Object arguments instead of two arguments and an options object

How do you feel about breaking your subview objects into an array? Then
the initialize method could just be for actual initialization, and not
configuration as well. I feel that would promote readability in a lot of
classes. I made a couple of other minor API changes. There's still a few
issues with this way, but I like the readability:

TorsoView.extend({
events : {
'click.openCollapsibleSection1' : function() {this.showChildView('collapsible-section-1')},
'click.openCollapsibleSection2' : function() {this.showChildView('collapsible-section-2')}
},
children: [
{
alias: 'collapsible-section-1', // allows us to edit the config later and change properties.
view: new CollapsibleSection(),
injectionSite: 'collapsible-section-1', // default to alias if not set?
events: {
'CLOSE': 'hide'
}
},
{
alias: 'collapsible-section-2', // allows us to edit the config later and change properties.
childView: new CollapsibleSection(),
listenTo: {
'CLOSE': 'hide'
}
},
{
alias: 'aside',
view: function() {return this._asideView;},
sharedView: true
},
{
alias: 'footer',
view: function() {return this._footerView;},
sharedView: true
}
],

initialize: function() {
this._asideView = new AsideView();
this._footerView = new FooterView();
},

changeAside: function(newAsideView) {
this._updateTrackedView('aside', {view: newAsideView});
}
});

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#238 (comment)

pepperbc · 2016-04-07T16:04:05Z

Initializing in initialize and then grabbing from the config I still find a bit awkward (as in aside and footer above). Agree that the static initialization is rarely going to be useful. I think you should be able to declare everything at the class level - I could see all of the options being valid as functions, so more complex logic is possible (either switching out the views or switching out the injection point)

kentmw · 2016-04-07T22:34:21Z

crazy thought: what if you used the alias as what to grab off the view?

TorsoView.extend({
  events : {
    'click.openCollapsibleSection1' : function() {this.showChildView('collapsible-section-1')},
    'click.openCollapsibleSection2' : function() {this.showChildView('collapsible-section-2')}
  },
  children: {
    collapsibleSection: {
      view: function() { return this.otherNamedView; }, // defaults to this[key]
      injectionSite: 'collapsible-section-1', // default to key if not set
      events: {
        'CLOSE': 'hideSection1'
      }
    },
    collapsibleSection2: {
      listenTo: {
        'CLOSE': 'hideSection2'
      }
    },
    aside: {
      sharedView: true
    },
    footer: {
      sharedView: true
    }
  },

  initialize: function(args) {
    this.aside = new AsideView();
    this.footer = new FooterView();
    this.otherNamedView = args.collapsibleSection || new CollapsibleSectionView(); // see custom view getter.
    this.collapsibleSection2 = args.collapsibleSection2 || new CollapsibleSectionView();
  },

  changeAside: function(newAsideView) {
    this.aside = newAsideView;
  }
});

Obviously, after thinking this through a bit, setting the views like this doesn't trigger anything and therefore doesn't rebind the "tracked" part until a re-render. But, I did like the feel of it.

mandragorn · 2016-04-08T13:28:29Z

We could follow a similar API as adding form model tracking (again thinking to keep this type of API consistent) and allow you to separate configuration (via alias) from setting the view.

I would expect 2 available ways to configure this (similar to form model binding):

Configuration at the class level. view can be either function or value (_.result()) and if not set then it will not 'activate' or 'use' the view until the parent class associates it with the alias.
Configuration via a configureChildView() method that you can call from anywhere - this gives flexibility to make decisions or change configurations of child view during the parent view's lifecycle.

The configurations above can define the view, but you can also change which view is associated with an alias (or initialize it to begin with) using an associateChildView() method (or an overload of configureChildView()?).

configureChildView() would go through the same process of determining the polymorphism that we did for #209.

pepperbc · 2016-04-08T16:59:38Z

Agree - anything that uses configuration at the class level should be overrideable programatically. Though I would find it awkward for the simple case if I had to specify the alias/injection point at the class level, and then set it in initialize as well. The class level wouldn't really be providing any additional information.

mandragorn · 2016-04-10T19:56:27Z

you wouldn't need to do both. Generally you would do one or the other. And the simple case should still be simple:

initialize: function() {
  this.configureTrackedView('injection-site', new SomeView());
}

That is what I mean by polymorphism and following the process we did for #209 where we came up with what were the most common use cases and made them easy to configure.

In all cases you can pass in the same configuration you would define at class level as a configuration object to configureTrackedView(configuration);

I think for this case the main use cases we need to configure are (please suggest more and better suggestions if you have them - I'm specifically optimizing brevity for common cases below):

Owned child view, always rendered:

Suggestions:

class level:

trackedViews: {
  simpleView: {
    injectionSite: 'injection-site',
    view: SomeView
  }
}

or, if you need to configure the view:

trackedViews: {
  simpleView: {
    injectionSite: 'injection-site',
    view: function() {
      return new SomeView(this._someContext);
    }
  }
}

in initialize(): `configureTrackedView('injection-site', new SomeView());

Shared child view, always rendered:
- Suggestions:
  - class level:
```
trackedViews: {
  simpleSharedView: {
    injectionSite: 'injection-site',
    view: sharedViewInstance,
    shared: true
  }
}
```
  - in initialize(): configureTrackedView('injection-site', true, someViewInstance);

Owned or shared view that has logic of whether to inject it or not:

Suggestions:

class level:

trackedViews: {
  simpleSharedView: {
    injectionSite: 'injection-site',
    shouldRender: function(showView) {
      return showView && this._shouldShowView();
    },
    view: function() {
      return new SimpleView(this._configuration);
    },
    shared: true/not-set
  }
}

What are other common cases?

Does it make sense to always return the alias when calling it programatically and generate it if one isn't specified?

pepperbc · 2016-04-11T17:55:40Z

A point of potential confusion: does a view function get evaluated once or every render. If you're switching between views in the same injection point you'd want every render. But if you wanted to instantiate things there, you'd only want once. I think it should only be evaluated once, and if you want to switch every render, you could put that sort of logic in preRender()

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify API for child views #238

Simplify API for child views #238

pepperbc commented Apr 4, 2016

mandragorn commented Apr 5, 2016

pepperbc commented Apr 6, 2016

pepperbc commented Apr 6, 2016

kentmw commented Apr 6, 2016

kentmw commented Apr 6, 2016

mandragorn commented Apr 7, 2016

mandragorn commented Apr 7, 2016

pepperbc commented Apr 7, 2016

kentmw commented Apr 7, 2016

pepperbc commented Apr 7, 2016

kentmw commented Apr 7, 2016

mandragorn commented Apr 8, 2016

pepperbc commented Apr 8, 2016

mandragorn commented Apr 10, 2016

pepperbc commented Apr 11, 2016

Simplify API for child views #238

Simplify API for child views #238

Comments

pepperbc commented Apr 4, 2016

mandragorn commented Apr 5, 2016

pepperbc commented Apr 6, 2016

pepperbc commented Apr 6, 2016

kentmw commented Apr 6, 2016

kentmw commented Apr 6, 2016

mandragorn commented Apr 7, 2016

mandragorn commented Apr 7, 2016

pepperbc commented Apr 7, 2016

kentmw commented Apr 7, 2016

pepperbc commented Apr 7, 2016

kentmw commented Apr 7, 2016

mandragorn commented Apr 8, 2016

pepperbc commented Apr 8, 2016

mandragorn commented Apr 10, 2016

pepperbc commented Apr 11, 2016