2

LIVE DEMO

Consider the following spinner-click directive:

Directive Use:

<button class="btn btn-mini"
        ng-class="{'btn-warning': person.active, disabled: !person.active}"
        spinner-click="deleteItem($index)"
        spinner-text="Please wait..."
        spinner-errors="alerts">
  Delete
</button>

Directive:

app.directive('spinnerClick', function() {
  return {
    restrict: 'A',
    scope: true,
    link: function(scope, element, attrs) {
      var originalHTML = element.html();
      var spinnerHTML = "<i class='icon-refresh icon-spin'></i> " + attrs.spinnerText;

      element.click(function() {
        if (element.is('.disabled')) {
          return;
        }

        element.html(spinnerHTML).addClass('disabled');

        scope.$apply(attrs.spinnerClick).then(function() {
          element.html(originalHTML).removeClass('disabled');
        }, function(errors) {
          element.html(originalHTML).removeClass('disabled');

          // This is ugly! Is there a better way?
          var e = scope[attrs.spinnerErrors];
          e.length = 0;
          e.push.apply(e, errors);
        });
      });
    }
  };
});

Controller:

app.controller('MainCtrl', function($scope, $q, $timeout) {
  $scope.alerts = ['First alert'];
  $scope.people = [
    { name: 'David', active: true },
    { name: 'Layla', active: false }
  ];

  $scope.deleteItem = function(index) {
    var defer = $q.defer();

    $timeout(function() {
      defer.reject(["Something 'bad' happened.", "Check your logs."]);
    }, 2000);

    return defer.promise;
  };
});

Note: spinner-click can be used with other directives (e.g. ng-class in this example).

As you can see, I set $scope.alerts in the directive using a very nasty way. Can you find a better way to do this?


UPDATE: (DEMO)

I tried to use $parse like this:

var errorsModel = $parse(attrs.spinnerErrors);
errorsModel.assign(scope, errors);

and this doesn't work.

BUT, if I have spinner-errors="wrapper.alerts" rather than spinner-errors="alerts", it does work!

Is there a way to avoid using the wrapper?

Misha Moroshko
  • 166,356
  • 226
  • 505
  • 746

5 Answers5

3

I think you can do it more simply using an isolate scope.

Instead of scope: true,, you should put:

scope:{
    spinnerClick:"&",
    spinnerText : "@",
    spinnerErrors: "="
 }

And then, in your directive use scope.spinnerClick, scope.spinnerText , scope.spinnerErrors directly.

The & is used to bind function expression defined in your attribute and pass it to your directive's scope, the @ will bind the text value of the attribute and the = will set a double binding with the expression passed in the attribute.

You can fine a more precise explanation here http://docs.angularjs.org/guide/directive (look at the long version), and a much clearer explanation here http://www.egghead.io/ (look at the isolate scope videos, it only takes a few minutes and makes it look so simple).

Yann
  • 2,211
  • 1
  • 15
  • 14
2

To answer your original question regarding the ugliness of

// This is ugly! Is there a better way?
var e = scope[attrs.spinnerErrors];
e.length = 0;
e.push.apply(e, errors);

You can use angular.copy to achieve the same results

angular.copy(errors, scope[attrs.spinnerErrors]);

The reason this is so ugly in your directive is due to your use of a child scope. If you did not create a child scope, or were willing to create an isolation scope this would not be a problem. You can't use $scope.alerts because

The child scope gets its own property that hides/shadows the parent property of the same name. Your workarounds are

  1. define objects in the parent for your model, then reference a property of that object in the child: parentObj.someProp
  2. use $parent.parentScopeProperty (not always possible, but easier than 1. where possible)
  3. define a function on the parent scope, and call it from the child (not always possible)

There is a detailed explanation that can be found here.

Community
  • 1
  • 1
rtcherry
  • 4,840
  • 22
  • 27
0

One option is to create a setter function in the controller that you can call in the directive. The reference to the function in the child scope could then be used set the value in the parent scope. Another option is to create an isolation scope and then pass in a setter function using & binding.

rtcherry
  • 4,840
  • 22
  • 27
  • Creating a setter function in every controller is not ideal. Also, isolate scope will be problematic since there are other directives on the same element. – Misha Moroshko Jul 07 '13 at 22:12
  • There is no need to repeat the setter functions in every controller. You could put them in `$rootScope` or use a mix-in like pattern (here is one [example](http://plnkr.co/edit/Fqrp5X?p=preview), but it may not match up 100% with what you are shooting for). There are several options for DRYing up your controllers. – rtcherry Jul 07 '13 at 22:55
0

You had the right idea with $parse. The problem is that you're assigning the new array of errors to the child scope, which hides (but doesn't replace) the array on the parent/controller scope.

What you have to do is get a reference to the parent array and then replace the contents (like you were doing in the original version). See here.

David Bennett
  • 1,885
  • 1
  • 12
  • 13
0

I question the need to put the error logic within the directive. You can simply handle the error as part of the controller. Unless you absolutely need to have the html replaced and class removed before manipulating the alerts array, your code could be rewritten as:

app.directive('spinnerClick', function() {
  return {
    restrict: 'A',
    scope: true,
    link: function(scope, element, attrs) {
      var originalHTML = element.html();
      var spinnerHTML = "<i class='icon-refresh icon-spin'></i> " + attrs.spinnerText;

      function onClickDone(){
        element.html(originalHTML).removeClass('disabled');
      }

      element.click(function() {
        if (element.is('.disabled')) {
          return;
        }

        element.html(spinnerHTML).addClass('disabled');

        scope.$apply(attrs.spinnerClick).then(onClickDone, onClickDone);
      });
    }
  };
});

app.controller('MainCtrl', function($scope, $q, $timeout) {
  $scope.alerts = ['First alert'];
  $scope.people = [
    { name: 'David', active: true },
    { name: 'Layla', active: false }
  ];

  $scope.deleteItem = function(index) {
    var defer = $q.defer();

    $timeout(function() {
      defer.reject(["Something 'bad' happened.", "Check your logs."]);
    }, 2000);

    return defer.promise.then(function(){
        //Success handler
    }, function(error){
        $scope.alerts.length = 0;
        $scope.alerts.push(error);
    });
  };
});
Clark Pan
  • 6,027
  • 1
  • 22
  • 18