Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

As I read it, you want:

  • one <div class="names"> per todo (in this case inside .contents) surrounding the todo text in a span and the buttons "Edit" / "Remove"
  • on clicking "Edit", the span is exchanged for an input field and an "Update" button, "Edit" and "Remove" are removed.
  • on clicking "Update", the prior state with the new text is recreated (span, Edit, Remove)

After you follow InkbugInkbug`s advice from his comment, you should probably do this:

  • cache the todo root element (var todoRoot = $(...);)

  • rename addUpdateField/.update to edit/.edit and updateBtn to update - consistent with the button texts and easier to read

  • change the api of your todo.js functions. For update / edit / remove, just use the dom node of the div as the argument:

     function(target, action) {
         todoRoot.on('click', target, function(e) {
             e.preventDefault();
             e.stopPropagation();
             action.call(todo, $(this).parents('.names').first());
         });
     }
    
     bindClick('.remove', todo.remove);
     bindClick('.edit',   todo.edit);
     bindClick('div',     todo.update);
    
  • don't use that many appends. Put it all in a string (that can contain more than one DOM node!)

  • don't create / delete all the nodes. Create all three buttons, use .toggleClass(...) and CSS to make all buttons/fields disappear that you don't need. That moves all your HTML into the add function and goes easy on the DOM

  • move the bindings into the Todo function. You don't want to have to remember to bind these events for every Todo list you create

As I read it, you want:

  • one <div class="names"> per todo (in this case inside .contents) surrounding the todo text in a span and the buttons "Edit" / "Remove"
  • on clicking "Edit", the span is exchanged for an input field and an "Update" button, "Edit" and "Remove" are removed.
  • on clicking "Update", the prior state with the new text is recreated (span, Edit, Remove)

After you follow Inkbug`s advice from his comment, you should probably do this:

  • cache the todo root element (var todoRoot = $(...);)

  • rename addUpdateField/.update to edit/.edit and updateBtn to update - consistent with the button texts and easier to read

  • change the api of your todo.js functions. For update / edit / remove, just use the dom node of the div as the argument:

     function(target, action) {
         todoRoot.on('click', target, function(e) {
             e.preventDefault();
             e.stopPropagation();
             action.call(todo, $(this).parents('.names').first());
         });
     }
    
     bindClick('.remove', todo.remove);
     bindClick('.edit',   todo.edit);
     bindClick('div',     todo.update);
    
  • don't use that many appends. Put it all in a string (that can contain more than one DOM node!)

  • don't create / delete all the nodes. Create all three buttons, use .toggleClass(...) and CSS to make all buttons/fields disappear that you don't need. That moves all your HTML into the add function and goes easy on the DOM

  • move the bindings into the Todo function. You don't want to have to remember to bind these events for every Todo list you create

As I read it, you want:

  • one <div class="names"> per todo (in this case inside .contents) surrounding the todo text in a span and the buttons "Edit" / "Remove"
  • on clicking "Edit", the span is exchanged for an input field and an "Update" button, "Edit" and "Remove" are removed.
  • on clicking "Update", the prior state with the new text is recreated (span, Edit, Remove)

After you follow Inkbug`s advice from his comment, you should probably do this:

  • cache the todo root element (var todoRoot = $(...);)

  • rename addUpdateField/.update to edit/.edit and updateBtn to update - consistent with the button texts and easier to read

  • change the api of your todo.js functions. For update / edit / remove, just use the dom node of the div as the argument:

     function(target, action) {
         todoRoot.on('click', target, function(e) {
             e.preventDefault();
             e.stopPropagation();
             action.call(todo, $(this).parents('.names').first());
         });
     }
    
     bindClick('.remove', todo.remove);
     bindClick('.edit',   todo.edit);
     bindClick('div',     todo.update);
    
  • don't use that many appends. Put it all in a string (that can contain more than one DOM node!)

  • don't create / delete all the nodes. Create all three buttons, use .toggleClass(...) and CSS to make all buttons/fields disappear that you don't need. That moves all your HTML into the add function and goes easy on the DOM

  • move the bindings into the Todo function. You don't want to have to remember to bind these events for every Todo list you create

added 1 characters in body
Source Link
Arne
  • 681
  • 4
  • 12

As I read it, you want:

  • one <div class="names"> per todo (in this case inside .contents) surrounding the todo text in a span and the buttons "Edit" / "Remove"
  • on clicking "Edit", the span is exchanged for an input field and an "Update" button, "Edit" and "Remove" are removed.
  • on clicking "Update", the prior state with the new text is recreated (span, Edit, Remove)

After you follow Inkbug`s advice from his comment, you should probably do this:

  • cache the todo root element (var todoRoot = $(...);)

  • rename addUpdateField/.update to edit/.edit and updateBtn to update - consistent with the button texts and easier to read

  • change the api of your todo.js functions. For update / edit / remove, exprectjust use the dom node of the div as the argument:

     function(target, action) {
         todoRoot.on('click', target, function(e) {
             e.preventDefault();
             e.stopPropagation();
             action.call(todo, $(this).parents('.names').first());
         });
     }
    
     bindClick('.remove', todo.remove);
     bindClick('.edit',   todo.edit);
     bindClick('div',     todo.update);
    
  • don't use that many appends. Put it all in a string (that can contain more than one DOM node!)

  • don't create / delete all the nodes. Create all three buttons, use .toggleClass(...) and CSS to make all buttons/fields disappear that you don't need. That moves all your HTML into the add function and goes easy on the DOM

  • move the bindings into the Todo function. You don't want to have to remember to bind these events for every Todo list you create

As I read it, you want:

  • one <div class="names"> per todo (in this case inside .contents) surrounding the todo text in a span and the buttons "Edit" / "Remove"
  • on clicking "Edit", the span is exchanged for an input field and an "Update" button, "Edit" and "Remove" are removed.
  • on clicking "Update", the prior state with the new text is recreated (span, Edit, Remove)

After you follow Inkbug`s advice from his comment, you should probably do this:

  • cache the todo root element (var todoRoot = $(...);

  • rename addUpdateField/.update to edit/.edit and updateBtn to update - consistent with the button texts and easier to read

  • change the api of your todo.js functions. For update / edit / remove, exprect the dom node of the div:

     function(target, action) {
         todoRoot.on('click', target, function(e) {
             e.preventDefault();
             e.stopPropagation();
             action.call(todo, $(this).parents('.names').first());
         });
     }
    
     bindClick('.remove', todo.remove);
     bindClick('.edit',   todo.edit);
     bindClick('div',     todo.update);
    
  • don't use that many appends. Put it all in a string (that can contain more than one DOM node!)

  • don't create / delete all the nodes. Create all three buttons, use .toggleClass(...) and CSS to make all buttons/fields disappear that you don't need. That moves all your HTML into the add function and goes easy on the DOM

  • move the bindings into the Todo function. You don't want to have to remember to bind these events for every Todo list you create

As I read it, you want:

  • one <div class="names"> per todo (in this case inside .contents) surrounding the todo text in a span and the buttons "Edit" / "Remove"
  • on clicking "Edit", the span is exchanged for an input field and an "Update" button, "Edit" and "Remove" are removed.
  • on clicking "Update", the prior state with the new text is recreated (span, Edit, Remove)

After you follow Inkbug`s advice from his comment, you should probably do this:

  • cache the todo root element (var todoRoot = $(...);)

  • rename addUpdateField/.update to edit/.edit and updateBtn to update - consistent with the button texts and easier to read

  • change the api of your todo.js functions. For update / edit / remove, just use the dom node of the div as the argument:

     function(target, action) {
         todoRoot.on('click', target, function(e) {
             e.preventDefault();
             e.stopPropagation();
             action.call(todo, $(this).parents('.names').first());
         });
     }
    
     bindClick('.remove', todo.remove);
     bindClick('.edit',   todo.edit);
     bindClick('div',     todo.update);
    
  • don't use that many appends. Put it all in a string (that can contain more than one DOM node!)

  • don't create / delete all the nodes. Create all three buttons, use .toggleClass(...) and CSS to make all buttons/fields disappear that you don't need. That moves all your HTML into the add function and goes easy on the DOM

  • move the bindings into the Todo function. You don't want to have to remember to bind these events for every Todo list you create

Source Link
Arne
  • 681
  • 4
  • 12

As I read it, you want:

  • one <div class="names"> per todo (in this case inside .contents) surrounding the todo text in a span and the buttons "Edit" / "Remove"
  • on clicking "Edit", the span is exchanged for an input field and an "Update" button, "Edit" and "Remove" are removed.
  • on clicking "Update", the prior state with the new text is recreated (span, Edit, Remove)

After you follow Inkbug`s advice from his comment, you should probably do this:

  • cache the todo root element (var todoRoot = $(...);

  • rename addUpdateField/.update to edit/.edit and updateBtn to update - consistent with the button texts and easier to read

  • change the api of your todo.js functions. For update / edit / remove, exprect the dom node of the div:

     function(target, action) {
         todoRoot.on('click', target, function(e) {
             e.preventDefault();
             e.stopPropagation();
             action.call(todo, $(this).parents('.names').first());
         });
     }
    
     bindClick('.remove', todo.remove);
     bindClick('.edit',   todo.edit);
     bindClick('div',     todo.update);
    
  • don't use that many appends. Put it all in a string (that can contain more than one DOM node!)

  • don't create / delete all the nodes. Create all three buttons, use .toggleClass(...) and CSS to make all buttons/fields disappear that you don't need. That moves all your HTML into the add function and goes easy on the DOM

  • move the bindings into the Todo function. You don't want to have to remember to bind these events for every Todo list you create