7

So I keep reading that jQuery manipulation from within a Controller is bad practice, but I'm not clear on why, or how to correct.

Below is code from a Youtube tutorial which even the video creator comments is a bad idea, but doesn't explain why and continues to use the bad behavior anyway.

From https://www.youtube.com/watch?v=ilCH2Euobz0#t=553s :

$scope.delete = function() {
    var id = this.todo.Id;
    Todo.delete({id: id}, function() {
        $('todo_' + id).fadeOut();
    });
};

SOLUTION:

Based on Langdon's answer below, I've arrived at the following working code for my own work, which derives slightly from the example code above:

var ProjectListCtrl = function ($scope, Project) {
    $scope.projects = Project.query();
    $scope.delete = function() {
        var thisElem = this;
        var thisProject = thisElem.project;
        var id = thisProject.id;
        Project.delete({id: id}, function() {
            var idx = $scope.projects.indexOf(thisProject);
            if (idx !== -1) {
                thisElem.destroy('removeItem('+idx+')');
            }
        });
    }

    $scope.removeItem = function(idx) {
        $scope.projects.splice(idx, 1);
    }

}

app.directive('fadeOnDestroy', function() {
    return function(scope, elem) {
        scope.destroy = function(funcComplete) {
            elem.fadeOut({
                complete: function() {
                    scope.$apply(funcComplete)
                }
            });
        }
    }
});

This differs from Langdon's answer in a few ways. I wanted to avoid adding a parameter to the ngClick callback, so I'm storing it in thisProject. Also, the example and my code needs to call destroy from within a $http success callback so instead of this which is no longer relevant, I'm storing the clicked element in thisElem.

UPDATE 2:

Updated my solution further to reflect that funcComplete was not actually modifying the original $scope.

DanH
  • 5,498
  • 4
  • 49
  • 72
  • 1
    There's no correct way to do DOM manipulations in Angular Controllers. This is what directives are for. See [How do I “think in AngularJS” if I have a jQuery background?](http://stackoverflow.com/a/15012542/1095616). – Stewie Apr 01 '13 at 14:36

2 Answers2

9

The Angular way to handle this is through a directive. I found a perfect example to cover what you're asking below, although it's not as clean as I'd like. The idea is that you create a directive to be used as an HTML attribute. When the element gets bound to the scope of your controller, the link function is fired. The function fades the element in (totally optional) and exposes a destroy method for your controller to call later.

Update: Modified based on comments to actually affect the scope. Not thrilled with the solution, and it's even jankier because the original author called complete.apply(scope) in his destroy callback, but doesn't use this inside the callback function.

Update 2: Since the directive is the one making the callback asynchronous, it's probably a better idea to use scope.$apply there, but keep in mind that that might get weird if you ever use isolated scope in your directive.

http://jsfiddle.net/langdonx/K4Kx8/114/

HTML:

<div ng-controller="MyCtrl">
  <ul>
      <li ng-repeat="item in items" fadey="500">
          {{item}}
          <a ng-click="clearItem(item)">X</a>
      </li>
  </ul>
  <hr />
  <button ng-click="items.push(items.length)">Add Item</button>    
</div>

JavaScript:

var myApp = angular.module('myApp', []);

//myApp.directive('myDirective', function() {});
//myApp.factory('myService', function() {});

function MyCtrl($scope) {
    $scope.items = [0, 1, 2];

    $scope.clearItem = function(item) {
        var idx = $scope.items.indexOf(item);
        if (idx !== -1) {
            //injected into repeater scope by fadey directive
            this.destroy(function() {
                $scope.items.splice(idx, 1);
            });
        }
    };
}

myApp.directive('fadey', function() {
    return {
        restrict: 'A', // restricts the use of the directive (use it as an attribute)
        link: function(scope, elm, attrs) { // fires when the element is created and is linked to the scope of the parent controller
            var duration = parseInt(attrs.fadey);
            if (isNaN(duration)) {
                duration = 500;
            }
            elm = jQuery(elm);
            elm.hide();
            elm.fadeIn(duration)

            scope.destroy = function(complete) {
                elm.fadeOut(duration, function() {
                    scope.$apply(function() {
                        complete.$apply(scope);
                    });
                });
            };
        }
    };
});

As for why, I think it's simply for separation of concerns and perhaps usability. Your controller should be concerned with data flow and business logic, not interface manipulation. You directives should ideally be written for usability (as in the case of fadey here -- ed. note: I wouldn't call it fadey ;)).

Langdon
  • 19,875
  • 18
  • 88
  • 107
  • 2
    if using jQuery, include it before angular.js so that angular then adopts the full jQuery library instead of it's own **[jQlite](http://docs.angularjs.org/api/angular.element)**. By doing this, `elm` in directive is a jQuery object already and don't have to wrap it in `jQuery(elm)` http://jsfiddle.net/ADukg/2222/ – charlietfl Apr 01 '13 at 14:54
  • I use console.log(this) on line 10 http://jsfiddle.net/K4Kx8/112/ it says this is a 'Child' object. why? what is this class? – Aladdin Mhemed Apr 01 '13 at 16:05
  • This all seems to work fine, however my original example includes an Object.delete success callback, within which `this` no longer refers to the element clicked, so I'm not sure what object I need to call `destroy()` on ? (also, apologies, I accepted the answer prematurely, until I realized it doesn't seem to cover the example above) – DanH Apr 01 '13 at 16:34
  • Ok figured it out, I'll post my exact solution and accept this. Thanks! – DanH Apr 01 '13 at 16:37
  • 1
    @AladdinHoms In this case, I believe it's the scope that exists inside the ng-repeat (where the item # and the destroy function are provided). – Langdon Apr 01 '13 at 17:15
  • 1
    there's a bit of a problem here actually, it seems the splicing doesn't affect the original $scope of the Controller. If I omit the `remove()` from my answer as you have, the LI still exists in the DOM albeit hidden by the `fadeOut()`. As a result, an "empty" row such as `
  • No items
  • ` fails to trigger since `items` continues to have length despite the splice. – DanH Apr 03 '13 at 18:21
  • Good catch! A simple fix is to wrap the splice in `$scope.$apply`, further convoluting the solution. I've updated the answer above. – Langdon Apr 03 '13 at 19:44
  • I've updated my solution, I went for a solution more in line with this: http://egghead.io/video/rough-draft-angularjs-directives-talking-to-contro/ – DanH Apr 04 '13 at 06:14