Add items collection

[tomgreenfield]

Original issue: #1637.
fixes:
#1365
#1178

Please test in conjuction with https://github.com/adaptlearning/adapt-contrib-accordion/tree/issue/1637.

[oliverfoster]

@lc-thomasberger what do you think?

[danielstorey]

I just started to test this using a new component for which I originally used itemsModel.

I understand the benefit of using a collection but now it feels a bit more laboured now as I can no longer use a single handlebars template to iterate over _items or refer to specific indices, and am having to write extra functions and create new templates in order to perform a simple initial render.

The issue is that .toJSON() returns a shallow copy of the model's attributes property and so _items remains a collection instance rather than being converted into an object literal readable by handlebars. Can we therefore redifine this method on the itemsComponentModel ie

toJSON: function() {
    var json = _.clone(this.attributes);
    json._items = this.get('_items').toJSON();
    return json;
},

With maybe some defensive programming to ensure we can call toJSON on the items collection.

You can see this is a simple extension of the original Backbone method here: https://github.com/jashkenas/backbone/blob/master/backbone.js#L439-L441

[oliverfoster]

Sounds fine to me. Need anything else?

[danielstorey]

Everything else works great!

[oliverfoster]

return early?

[tomgreenfield]

Aha, I think defiite overkill / needless expansion for single-line if statements. Only an issue if dealing with multiple nested lines and/or actions after the conditional.

[brian-learningpool]

You could actually call .map() here and call the Collection add() with an array of items after.

[oliverfoster]

can you write out the code? my brain has gone to sleep... :/

[brian-learningpool]

Something like this:

var items = items.map(function(item, index) {
  item._index = index;
  return item;
});

collection.add(items);

[danielstorey]

This doesn't make use of the array returned by calling .map so .forEach could still be used. If immutability is desirable though
var items = items.slice().map(
would prevent mutation

Also since there's now only one .add call why not just use

var collection = new Backbone.Collection(items, { model: itemModel })

After the array has been mapped.

Sorry for the lack of MD I'm on a phone currently.

[brian-learningpool]

Good point.

[brian-learningpool]

Would be more readable on a single line if we were using arrow functions.

[oliverfoster]

:O * gasp *

[brian-learningpool]

Can't do that with IE 11 still in the mix though. :-/

[oliverfoster]

I need to have a chat with you about that kind of stuff.

[oliverfoster]

could you all re-+1 this so it can go in?

[brian-learningpool]

Nice job, @tomgreenfield. 👍