21
\$\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.

asked Sep 3, 2014 at 13:24
\$\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\$ Commented Sep 10, 2014 at 19:58
  • \$\begingroup\$ this jfiddle do not work in ff 44.0.2 \$\endgroup\$ Commented Feb 23, 2016 at 11:03
  • \$\begingroup\$ @ghdj My bad, I updated the JSFiddle. \$\endgroup\$ Commented Feb 23, 2016 at 11:28

1 Answer 1

11
+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 ] );
 };
 });
})();
answered Sep 11, 2014 at 14:08
\$\endgroup\$
4
  • \$\begingroup\$ But where exactly was the bug? Also, have you seen this? \$\endgroup\$ 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\$ 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\$ 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\$ Commented Sep 14, 2014 at 14:44

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.