20
\$\begingroup\$

This is my first angular code. I've been working with jQuery for a while, so I don't have the same approach. I'm looking for advice and code improvement.

The following code has 3 directives:

  • ceBoxCreator: Will create a div that will be draggable and resizable on a click.

  • ceDrag: The element binded to this directive will be draggable.

  • ceResize: The element binded to this directive will be resizable.

(function() {
    var contentEditor = angular.module("contentEditor", []);

    // To create a empty resizable and draggable box
    contentEditor.directive("ceBoxCreator", function($document, $compile) {
        return {
            restrict: 'A',
            link: function($scope, $element, $attrs) {
                $element.on("mousedown", function($event) {
                    var newNode = $compile('<div class="contentEditorBox" ce-drag ce-resize></div>')($scope);

                    newNode.css({
                        position: "absolute",
                        top: $event.pageY - 25 + "px",
                        left: $event.pageX - 25 + "px",
                        });

                    angular.element($document[0].body).append(newNode);
                });
            }
        }
    });

    // To manage the drag
    contentEditor.directive("ceDrag", function($document) {
        return function($scope, $element, $attr) {
            var startX = 0, startY = 0;
            var newElement = angular.element('<div class="draggable"></div>');

            $element.append(newElement);
            newElement.on("mousedown", function($event) {
                $event.preventDefault();

                // To keep the last selected box in front
                angular.element(document.querySelectorAll(".contentEditorBox")).css("z-index", "0");
                $element.css("z-index", "1"); 

                startX = $event.pageX - $element[0].offsetLeft;
                startY = $event.pageY - $element[0].offsetTop;
                $document.on("mousemove", mousemove);
                $document.on("mouseup", mouseup);
            });

            function mousemove($event) {
                y = $event.pageY - startY;
                x = $event.pageX - startX;
                $element.css({
                    top: y + "px",
                    left:  x + "px"
                });
            }

            function mouseup() {
                $document.off("mousemove", mousemove);
                $document.off("mouseup", mouseup);
            }
          };
        });

    // To manage the resize
    contentEditor.directive("ceResize", function($document) {
        return function($scope, $element, $attr) {

            // Function to manage resize up event
            var resizeUp = function($event) {
                var top = $event.pageY;
                var height = $element[0].offsetTop + $element[0].offsetHeight - $event.pageY;

                if ($event.pageY < $element[0].offsetTop + $element[0].offsetHeight - 50) {
                    $element.css({
                        top: top + "px",
                        height: height + "px"
                    });
                } else {
                    $element.css({
                        top: $element[0].offsetTop + $element[0].offsetHeight - 50 + "px",
                        height: "50px"
                    }); 
                }
            };

            // Function to manage resize right event
            var resizeRight = function($event) {
                var width = $event.pageX - $element[0].offsetLeft;

                if ($event.pageX > $element[0].offsetLeft + 50) {
                    $element.css({
                        width: width + "px"
                    });
                } else {
                    $element.css({
                        width: "50px",
                    });
                }
            };

            // Function to manage resize down event
            var resizeDown = function($event) {
                var height = $event.pageY - $element[0].offsetTop;

                if ($event.pageY > $element[0].offsetTop + 50) {
                    $element.css({
                        height: height + "px"
                    });
                } else {
                    $element.css({
                        height: "50px"
                    });
                }
            };

            // Function to manage resize left event
            var resizeLeft = function($event) {
                var left = $event.pageX;
                var width = $element[0].offsetLeft + $element[0].offsetWidth - $event.pageX;

                if ($event.pageX < $element[0].offsetLeft + $element[0].offsetWidth - 50) {
                    $element.css({
                        left: left + "px",
                        width: width + "px"
                    });
                } else {
                    $element.css({
                        left: $element[0].offsetLeft + $element[0].offsetWidth - 50 + "px",
                        width: "50px"
                    });
                }
            };

            // Create a div to catch resize up event
            var newElement = angular.element('<div class="n-resize"></div>');
            $element.append(newElement);
            newElement.on("mousedown", function() {
                $document.on("mousemove", mousemove);
                $document.on("mouseup", mouseup);

                function mousemove($event) {
                    $event.preventDefault();
                    resizeUp($event);
                }

                function mouseup() {
                    $document.off("mousemove", mousemove);
                    $document.off("mouseup", mouseup);
                }
            });

            // Create a div to catch resize right event
            newElement = angular.element('<div class="e-resize"></div>');
            $element.append(newElement);
            newElement.on("mousedown", function() {
                $document.on("mousemove", mousemove);
                $document.on("mouseup", mouseup);

                function mousemove($event) {
                    $event.preventDefault();
                    resizeRight($event);
                }

                function mouseup() {
                    $document.off("mousemove", mousemove);
                    $document.off("mouseup", mouseup);
                }
            });

            // Create a div to catch resize down event
            newElement = angular.element('<div class="s-resize"></div>');
            $element.append(newElement);
            newElement.on("mousedown", function() {
                $document.on("mousemove", mousemove);
                $document.on("mouseup", mouseup);

                function mousemove($event) {
                    $event.preventDefault();
                    resizeDown($event);
                }

                function mouseup() {
                    $document.off("mousemove", mousemove);
                    $document.off("mouseup", mouseup);
                }
            });

            // Create a div to catch resize left event
            newElement = angular.element('<div class="w-resize"></div>');
            $element.append(newElement);
            newElement.on("mousedown", function() {
                $document.on("mousemove", mousemove);
                $document.on("mouseup", mouseup);

                function mousemove($event) {
                    $event.preventDefault();
                    resizeLeft($event);
                }

                function mouseup() {
                    $document.off("mousemove", mousemove);
                    $document.off("mouseup", mouseup);
                }
            });

            // Create a div to catch resize up left event
            newElement = angular.element('<div class="nw-resize"></div>');
            $element.append(newElement);
            newElement.on("mousedown", function() {
                $document.on("mousemove", mousemove);
                $document.on("mouseup", mouseup);

                function mousemove($event) {
                    $event.preventDefault();
                    resizeUp($event);
                    resizeLeft($event);
                }

                function mouseup() {
                    $document.off("mousemove", mousemove);
                    $document.off("mouseup", mouseup);
                }
            });

            // Create a div to catch resize up right event
            newElement = angular.element('<div class="ne-resize"></div>');
            $element.append(newElement);
            newElement.on("mousedown", function() {
                $document.on("mousemove", mousemove);
                $document.on("mouseup", mouseup);

                function mousemove($event) {
                    $event.preventDefault();
                    resizeUp($event);
                    resizeRight($event);
                }

                function mouseup() {
                    $document.off("mousemove", mousemove);
                    $document.off("mouseup", mouseup);
                }
            });

            // Create a div to catch resize down right event
            newElement = angular.element('<div class="se-resize"></div>');
            $element.append(newElement);
            newElement.on("mousedown", function() {
                $document.on("mousemove", mousemove);
                $document.on("mouseup", mouseup);

                function mousemove($event) {
                    $event.preventDefault();
                    resizeDown($event);
                    resizeRight($event);
                }

                function mouseup() {
                    $document.off("mousemove", mousemove);
                    $document.off("mouseup", mouseup);
                }
            });

            // Create a div to catch resize down left event
            newElement = angular.element('<div class="sw-resize"></div>');
            $element.append(newElement);
            newElement.on("mousedown", function() {
                $document.on("mousemove", mousemove);
                $document.on("mouseup", mouseup);

                function mousemove($event) {
                    $event.preventDefault();
                    resizeDown($event);
                    resizeLeft($event);
                }

                function mouseup() {
                    $document.off("mousemove", mousemove);
                    $document.off("mouseup", mouseup);
                }
            });
        };
    });
})();

To see what the code does: jsFiddle

For example:

  • There is code repetition on the ceResize directive of the mousemove and mouseup event. If I put this function at the root of the directive, it cannot be accessed because it's not the correct scope.

  • There is HTML code in the JS file. Is it possible to extract it and keep the events available?

  • I've heard of things like injector, scope, link (in directives), templateURL, etc... But those are still vague concepts to me. And it seems like there is multiple ways to do the same thing. But is there "best ways" to use Angular ?

  • Also why is there two different prototypes for a directive :

One with a function that returns a JSON object :

module.directive(directiveName, function(/* Some attributes (services?) */) {
    return {
        /* Some JSON attributes */
    }
});

And another one that returns a function :

module.directive(directiveName, function(/* Some attributes (services?) */) {
    return function(/* Some attributes (services?) */) {
        /* Some attributes and functions to handle events */
    }
});

This are the points I can think of, but feel free to review the code.

Note: I don't know why using JSFiddle the resize up and left are weird. It's working great on my computer. Anyway, this isn't the point here, just help me to understand how we are supposed to code using AngularJS.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Great question, just wanted to say that I see the same behaviour on jsbin.com, so I suspect your code is at fault, not jsfiddle. jsbin.com/sopapo/1/edit \$\endgroup\$
    – konijn
    Commented Sep 10, 2014 at 19:58
  • \$\begingroup\$ this jfiddle do not work in ff 44.0.2 \$\endgroup\$
    – user98482
    Commented Feb 23, 2016 at 11:03
  • \$\begingroup\$ @ghdj My bad, I updated the JSFiddle. \$\endgroup\$
    – Elfayer
    Commented Feb 23, 2016 at 11:28

1 Answer 1

10
+50
\$\begingroup\$

From a once over:

  • It makes more sense from a UX experience to create the box on click then on mousedown
  • There are a number of indenting inconsistencies, use TidyUp in jsFiddle to fix that
  • Naming is good
  • Flow of the code is good
  • Commenting is good
  • I don't like the constants 25 and 50 all over the place, name them and manage them
  • You have a few place where you can apply DRY

    • You set the top and left a number of times, simply create a helper function

      function placeNode(node, top, left) {
        node.css({
          position: "absolute",
          top: top + "px",
          left: left + "px",
        });
      }
      
    • You set css values a number of times by comparing values

      var width = $event.pageX - $element[0].offsetLeft;
      
      if ($event.pageX > $element[0].offsetLeft + 50) {
          $element.css({
             width: width + "px"
          });
      } else {
          $element.css({
              width: "50px",
           });
      }
      

      It is better to first deduce the value of width and then set it in 1 go:

      var margin = 50,
          leftest = $element[0].offsetLeft + margin,
          width = $event.pageX > leftest ? $event.pageX - $element[0].offsetLeft : margin;
      $element.css({
        width: width + "px"
      });
      
    • There is a ton of copy pastage when the resize divs are generated, use 1 common function that can generate a div with a class name and event handler(s).

All in all I thought your code looked okay, if repetitive, so I took a stab at both fixing your code ( for upper and left resize ) and making it less repetitive ( you can find this in action here ):

(function() {
  var contentEditor = angular.module("contentEditor", []);

  function placeNode(node, top, left) {
    node.css({
      position: "absolute",
      top: top + "px",
      left: left + "px",
    });
  }

  // To create a empty resizable and draggable box
  contentEditor.directive("ceBoxCreator", function($document, $compile) {
    return {
      restrict: 'A',
      link: function($scope, $element, $attrs) {
        $element.on("click", function($event) {

          var newNode = $compile('<div class="contentEditorBox" ce-drag ce-resize></div>')($scope);
          placeNode(newNode, $event.pageY - 25, $event.pageX - 25);
          angular.element($document[0].body).append(newNode);
        });
      }
    }
  });

  // To manage the drag
  contentEditor.directive("ceDrag", function($document) {
    return function($scope, $element, $attr) {
      var startX = 0,
          startY = 0;

      var newElement = angular.element('<div class="draggable"></div>');

      $element.append(newElement);
      newElement.on("mousedown", function($event) {
        event.preventDefault();

        // To keep the last selected box in front
        angular.element(document.querySelectorAll(".contentEditorBox")).css("z-index", "0");
        $element.css("z-index", "1");

        startX = $event.pageX - $element[0].offsetLeft;
        startY = $event.pageY - $element[0].offsetTop;
        $document.on("mousemove", mousemove);
        $document.on("mouseup", mouseup);
      });

      function mousemove($event) {
        placeNode( $element , $event.pageY - startY , $event.pageX - startX );
      }

      function mouseup() {
        $document.off("mousemove", mousemove);
        $document.off("mouseup", mouseup);
      }
    };
  });

  // To manage the resizers
  contentEditor.directive("ceResize", function($document) {
    return function($scope, $element, $attr) {
      //Reference to the original 
      var $mouseDown; 

      // Function to manage resize up event
      var resizeUp = function($event) {
        var margin = 50,
            lowest = $mouseDown.top + $mouseDown.height - margin,
            top = $event.pageY > lowest ? lowest : $event.pageY,
            height = $mouseDown.top - top + $mouseDown.height;

        $element.css({
          top: top + "px",
          height: height + "px"
        });
      };

      // Function to manage resize right event
      var resizeRight = function($event) {
        var margin = 50,
            leftest = $element[0].offsetLeft + margin,
            width = $event.pageX > leftest ? $event.pageX - $element[0].offsetLeft : margin;

        $element.css({
          width: width + "px"
        });
      };

      // Function to manage resize down event
      var resizeDown = function($event) {
        var margin = 50,
            uppest = $element[0].offsetTop + margin,
            height = $event.pageY > uppest ? $event.pageY - $element[0].offsetTop : margin;

        $element.css({
          height: height + "px"
        });
      };

      // Function to manage resize left event
      function resizeLeft ($event) {
        var margin = 50,
            rightest = $mouseDown.left + $mouseDown.width - margin,
            left = $event.pageX > rightest ? rightest : $event.pageX,
            width = $mouseDown.left - left + $mouseDown.width;        

        $element.css({
          left: left + "px",
          width: width + "px"
        });
      };

     var createResizer = function createResizer( className , handlers ){

        newElement = angular.element( '<div class="' + className + '"></div>' );
        $element.append(newElement);
        newElement.on("mousedown", function($event) {

          $document.on("mousemove", mousemove);
          $document.on("mouseup", mouseup);

          //Keep the original event around for up / left resizing
          $mouseDown = $event;
          $mouseDown.top = $element[0].offsetTop;
          $mouseDown.left = $element[0].offsetLeft
          $mouseDown.width = $element[0].offsetWidth;
          $mouseDown.height = $element[0].offsetHeight;                

          function mousemove($event) {
            event.preventDefault();
            for( var i = 0 ; i < handlers.length ; i++){
              handlers[i]( $event );
            }
          }

          function mouseup() {
            $document.off("mousemove", mousemove);
            $document.off("mouseup", mouseup);
          }         
        });
      }

      createResizer( 'sw-resize' , [ resizeDown , resizeLeft ] );
      createResizer( 'ne-resize' , [ resizeUp   , resizeRight ] );
      createResizer( 'nw-resize' , [ resizeUp   , resizeLeft ] );
      createResizer( 'se-resize' , [ resizeDown ,  resizeRight ] );
      createResizer( 'w-resize' , [ resizeLeft ] );
      createResizer( 'e-resize' , [ resizeRight ] );
      createResizer( 'n-resize' , [ resizeUp ] );
      createResizer( 's-resize' , [ resizeDown ] );
    };

  });
})();
\$\endgroup\$
4
  • \$\begingroup\$ But where exactly was the bug? Also, have you seen this? \$\endgroup\$
    – RubberDuck
    Commented Sep 11, 2014 at 14:19
  • 1
    \$\begingroup\$ @RubberDuck The bug is ignoring the original mousedown event. I would do something fancier for that challenge ;) \$\endgroup\$
    – konijn
    Commented Sep 11, 2014 at 14:24
  • \$\begingroup\$ Thanks for answering ! Do you have some advice about the architecture of the directives (directive prototype, link, scope, templateUrl, etc) ? \$\endgroup\$
    – Elfayer
    Commented Sep 12, 2014 at 15:30
  • \$\begingroup\$ @Elfayer I am no great Angular expert, all I can tell you is that your Angular code is understandable/maintainable and has no obvious flaws. \$\endgroup\$
    – konijn
    Commented Sep 14, 2014 at 14:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.