cbcrc / knockout-dragula

Simple binding for Knockout.js to add drag and drop functionality to observableArrays using dragula
13 stars 6 forks source link

afterDrop is now bound to context can optionally be set through `bind:` #6

Closed mblarsen closed 8 years ago

mblarsen commented 8 years ago

This PR implements a default binding of the afterDrop method to the elements binding context. This is to give the developer access to mainly the $parent object, since a common use case is to have a list model with a number of item models in it.

class ListViewModel () {
  constructor () {
    this.items = ko.observableArray([]);
  }

  afterDrop () {
    // this would otherwise be `undefined`
    for ([idx, item] of this.$parent.items().entries()) {
      item.sortOrder(idx);
    }
  }
}

In the example above this would normally be undefined. A more natural bind value would be to bind it to $parent since that is the object in which the afterDrop method is defined and you'll be able to use this as you would in other class methods / object functions. But since this is not always the scenario I've left it as an option (see README.md):

<div data-bind="dragula: { data: items, afterDrop: afterDrop, bind: '$parent' }"></div>

Doing so will set this to be the "real" this of afterDrop.

mblarsen commented 8 years ago

This is an option as well, but a default would be nice:

<div data-bind="dragula: { data: categories, afterDrop: afterDrop.bind($data) }">
laurentlbm-rc commented 8 years ago

Thanks for the pull request. I like the change to bind afterDrop by default, but I'm not sure the bind option is necessary. I personally find that afterDrop.bind($data) is enough. I still want to merge the whole pull request, though, since it adds flexibility. But, before I do, could you make the same changes for the afterDelete callback? Should the bind option apply to both or should they each have an option to set their bind value?

mblarsen commented 8 years ago

Yes afterDrop.bind($data) with a default binding would do fine. I do see the idea of binding to both, however I think it could lead to confusing as well if it is the same "class" of view model. What about just binding to the context then? Then you could get this.$data, this.$parent or this.$root even? @laurentlbm-rc

mblarsen commented 8 years ago

@laurentlbm-rc I've updated the code so that the bind-option is removed and it defaults to binding-context. Updated the README.md as well.

laurentlbm-rc commented 8 years ago

Looks good to me, thank you very much @mblarsen!