CODEWITHSUNDEEP
javascriptangularjsangularjs-directive
pardie
pardie

Reputation: 509

Improve AngularJS directive code

I wrote an AngularJS directive, but I'm pretty new about it, and I don't know if I done in the "Angular way"...

Here is my plunker with the code: http://plnkr.co/edit/X1tOk4z8f6dCK3mfB7HP?p=preview

html:

<!DOCTYPE html>
<html ng-app="app">

<head>
<script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.4.8/angular.min.js"></script>
<meta charset=utf-8 />
<title>Directive Test</title>
<script src="script.js"></script>
</head>

<body ng-controller="MainCtrl">

  <button id="button1" ng-click="dummyClickFoo()" wait-button="foo"><i></i> Foo</button>
  <button id="button2" ng-click="dummyClickBar()" wait-button="bar"><i></i> Bar</button>

</body>

</html>

js:

app = angular.module('app', []);

app.controller('MainCtrl', function($scope) {
    $scope.dummyClickFoo = function() {
        $scope.startSpinner('foo');

        setTimeout(function() {
                $scope.stopSpinner('foo');
          }, 3000);
    };

    $scope.dummyClickBar = function() {
        $scope.startSpinner('bar');

        setTimeout(function() {
                $scope.stopSpinner('bar');
          }, 3000);
    };
});


app.directive('waitButton', function() {
    return {
        restrict: 'A',
        controller: ['$scope', '$element', function($scope, $element) {
            $scope.startSpinner = function(id) {
                var el = angular.element(document.querySelector('[wait-button="'+id+'"]'));
                el.children('i').text('searching...');
            };

            $scope.stopSpinner = function(id) {
                var el = angular.element(document.querySelector('[wait-button="'+id+'"]'));
                el.children('i').empty();
            };
        }]
    };
});

I find that the document.querySelector('[wait-button="'+id+'"]') part, it's a bit "nasty"... (or not?); otherwise I don't know a better way to re-use the same directive different times in the same controller. Can someone suggest me a better code?

Thank you.

Upvotes: 3

Views: 130

Answers (4)

Justin
Justin

Reputation: 1120

I strongly suggest you convert 

<button id="button1" ng-click="dummyClickFoo()" wait-button="foo"><i></i> Foo</button>

into a directive, so your code will be 

<my-button  label="foo"></my-button>

Upvotes: 1

AdityaParab
AdityaParab

Reputation: 7100

The whole idea of directive is to facilitate separation of concerns.

When you use directive, all it's functionality should be taken care of by the directive itself.

Right now, you have part of the directive's logic in your MainCtrl which should be there ideally in your directive.

Also, in your directive, there is a controller function. Ideally, the controller function should only handle data passing between the views models etc.

For DOM manilulations, something that you are doing, it should be implemented in a link function.

So, the angular way of doing the same thing would be as follows.

Your Html

<body ng-controller="MainCtrl">

  <button id="button1" wait-button="foo"><i></i> Foo</button>
  <button id="button2" wait-button="bar"><i></i> Bar</button>
  <!-- NOTICE: No ng-click handlers here -->
</body>

Your MainCtrl

app.controller('MainCtrl', function($scope) {
    // No code required here. This will be handled in Directive's link function.
});

Your waitButton directive.

    app.directive('waitButton', function() {
        return {
            restrict: 'A',
            controller: ['$scope', '$element', function($scope, $element) {
//Again, it's not controller's job to handle DOM manipulations. So code required here.

            }],
            link: function($scope, $element){
              var waitButtons = angular.element(document.querySelectorAll('[wait-button]'));// NOTE: I've used querySelectorAll instead of querySelector. This will make this function generic.
              waitButtons.on('click', function(){
                var $this = angular.element(this);
                startSpinner($this);
                setTimeout(function(){
                  stopSpinner($this);
                },3000);
              });

               function startSpinner(el) {
                    el.children('i').text('searching...');
                }

               function stopSpinner(el) {
                    el.children('i').empty();
                }

            }
        };
    });

Upvotes: 1

jusopi
jusopi

Reputation: 6813

accessing the element in the directive

I'd advocate using the link function for this type of thing:

link: function($scope, elem, attrs){ /* do something w. elem */ }

It's not very angular-ish to be accessing your element in the controller. That's the whole point of the link & compile functions of the directive object....

... but in rare cases this is justified. The injected $element in your controller references the same thing your angular.element(document.querySelector('[wait-button="'+id+'"]')) code is doing. You only need to use $element at this point. But may I recommend a more angular approach altogether?

communicating betw. controllers and directives

The other issue is how you're basically communicating your intent from the directive to the main controller and back to the directive. Your use case is a little different than most in that you have an async nature.

I have made an example that leverages an isolate scope and callback parameters. In most real-world scenarios you'll be dealing with promises for async callbacks. As such, I used the .finally logic from the promises to execute the callback that communicates back to the directive that whatever async logic has wrapped up.

Things to keep in mind in my examples:

  • I used coffeescript because I code sanely that way
  • I used CSS/DOM to drive how the loading state of the directive should appear rather than trying to do it programmatically. Programmatic DOM manipulation is very much anti-NG in my book. Directives provide you with enough to do things like this declaratively.
  • I didn't use the directive controller because unless you're going to use a template for your directive, you really don't need a custom controller. There is a fuzzy line of when you go with a link function versus a custom directive controller.
  • Oh... and I used the *controller as• syntax because if you read anything about where NG is going, they are moving away from the whole $scope paradigm.

plunker - http://plnkr.co/edit/0AvlCQW5qqkpYKl2WpB3?p=preview

declaritive ui

main controller

.controller 'MainCtrl', class MainCtrl
  @$inject = [
    '$scope'
    '$interval'
  ]

  constructor: ($scope, @$interval)->
    @viewData = 'Skynet 2.0'
    @isLoading = false

  callbackExample: ($callbackFunc)->
    @loadRqst()
    .finally -> $callbackFunc?()

  loadRqst: ->
    @isLoading = 1
    # this returns a promise which gets processed in the example functions
    @$interval => 
      console.log @isLoading++
    , 250, 10 
    .finally => 
      @isLoading = false

implementing ui

<button callback-btn="vc.callbackExample($callbackFunc)">
  Callback Example<i> - I'm loading & I'm #1</i>
</button>

<button callback-btn="vc.callbackExample($callbackFunc)">
  Callback Example<i> - Look I can load too, I'm #2</i>
</button>

css

[callback-btn] i{
  display: none;
}

[callback-btn].loading i{
  display: initial;
}

the directive

.directive 'callbackBtn', ($parse)->
  dir =
    restrict: 'A' 
    scope: { callbackBtn: '&' }
    link: ($scope, elem, attrs)->

      onCallback = ->
        console.log 'on callback'
        elem.removeClass 'loading'

      elem.on 'click', ->
        elem.addClass 'loading'
        $scope.$apply ->
          $scope.callbackBtn({$callbackFunc: onCallback})

Upvotes: 2

georgeawg
georgeawg

Reputation: 48968

Move your functions into the directive. Put the click handler in the linking function.

app.directive('waitButton', function($timeout) {
    return {
        restrict: 'A',
        link: function (scope, elem, attrs) {
                   var startSpinner = function() {
                       elem.children('i').text('searching...');
                   };
                   var stopSpinner = function() {
                       elem.children('i').empty();
                   };
                   var clickHandler = function() {
                       startSpinner();
                       $timeout(stopSpinner,3000);
                   };
                   elem.on("click", clickHandler);
               }
     }
});

Also use the AngularJS $timeout service. For more information on that see the AngularJS $timeout service API Reference.

Upvotes: 0

Related Questions