jQuery module to edit HTML elements

Question

I need to turn a lot of html elements as editable elements. You will double click on the element to edit, press enter to validate (send a PUT to the RESTapi), press esc to undo.

I've written this piece of code. Are there any performance improvements to be made?

var editable = (function() {

    var DELAY = 300,
        clicks = 0,
        timer = null;

    var addEditDelegateEvents = function(element, selector, url, idInUrl, idAttr, ajaxAttr) {
        // disable the double click, and handle it in the single click envent
        element.on("dblclick", selector, function(event) {
            event.preventDefault();
        });
        element.on("click", selector, function(event) {
            event.preventDefault();
            var href = $(this).attr('href');
            clicks++; //count clicks

            if (clicks === 1) { // single click action
                timer = setTimeout(function() {
                    // For some browsers, `attr` is undefined; for others,
                    // `attr` is false.  Check for both.
                    if (typeof href !== typeof undefined && href !== false) {
                        window.location = href // follow hlink
                    };
                    clicks = 0; //after action performed, reset counter
                }, DELAY);

            } else { //double click action
                clearTimeout(timer); //prevent single-click action                
                // double click action
                //transform the double clicked element into an <input>
                var originElement = $(this);
                var editElement = $(document.createElement('input'));
                editElement.val(originElement.text());
                originElement.replaceWith(editElement);
                editElement.focus();
                id = originElement.attr(idAttr);
                url = url.replace(idInUrl, id); //replace idInUrl in url by id


                editElement.on("keypress", function(event) {

                    if (event.which == 13) {
                        //send ajax call then re-populate the list
                        var newVal = editElement.val();
                        var obj = {};
                        obj[ajaxAttr] = newVal;
                        $.ajax({
                            method: "PUT",
                            url: url,
                            data: JSON.stringify(obj),
                            contentType: "application/json; charset=utf-8",
                            success: function(data) {
                                console.log('sheet updated');
                                // Very careful here : the RESTapi should return a JSON
                                // with the updated infos
                                originElement.text(data[ajaxAttr]);
                                editElement.replaceWith(originElement);

                            },
                            error: function() {
                                console.log('error on sheet update');
                                editElement.replaceWith(originElement);
                            }
                        });
                    }
                    if (event.which == 27) {
                        editElement.replaceWith(originElement);
                    }
                });
                clicks = 0; //after action performed, reset counter
            }

        });

    };
    // public API
    return {
        addEditDelegateEvents: addEditDelegateEvents
    }
})();

And I will register my events as:

editable.addEditDelegateEvents($("p.editable_sheet_date_end"), "span", "/api/v1/sheets/ID/update", "ID", "sheet_id", "end");

I'm aware of the weakness of my implementation of url,idInUrl,idAttr, ajaxAttr.

But my question is rather: is this a solid design? My primary goal is the speed (loading and interactions).

I will have around 1000 delegates to deal with.

Answer 1

For your module definition, I suggest the following:

;(function(ns){

  function internalFunction(...){...}

  function anotherInternalFunction(...){...}

  ns.addDelegateEvents = function(...){...};

  ns.anotherFunction = function(...){...};

  ns.yetAnotherFunction = function(...){...};

}(this.editable = this.editable || {}));

This is more from the readability standpoint and not at all different from your approach. Your closure gets a reference to an existing global or creates one. Then you simply attach to it. You don't have to define this huge object at the end of the module.

---

            editElement.on("keypress", function(event) {

                if (event.which == 13) {
                    //send ajax call then re-populate the list
                    var newVal = editElement.val();
                    var obj = {};
                    obj[ajaxAttr] = newVal;
                    $.ajax({
                        method: "PUT",
                        url: url,
                        data: JSON.stringify(obj),
                        contentType: "application/json; charset=utf-8",
                        success: function(data) {
                            console.log('sheet updated');
                            // Very careful here : the RESTapi should return a JSON
                            // with the updated infos
                            originElement.text(data[ajaxAttr]);
                            editElement.replaceWith(originElement);

                        },
                        error: function() {
                            console.log('error on sheet update');
                            editElement.replaceWith(originElement);
                        }
                    });
                }
                if (event.which == 27) {
                    editElement.replaceWith(originElement);
                }
            });

Separation of concerns. Don't mix DOM code with AJAX code.

Move the AJAX into another function. Make it return a promise, which the caller can listen to. The DOM operation remains in the handler. It should be something like:

// This block of code is all DOM-related, with the exception of that AJAX
// function call. The AJAX code is moved out.
editElement.on("keypress", event => {

  if(event.which === 13){
    return theAjaxFunction(...).then(data => {
      // DOM operation on success
    }, error => {
      // DOM operation on error
    });
  }
});

// Meanwhile, somewhere else in your module
function theAjaxFunction(...){
  return $ajax(...);
}

---

if (event.which == 13) {
    ...
}
if (event.which == 27) {
    ...
}

To avoid foot-guns, put an else to avoid JS having to evaluate the second condition. One issue that might arise here is when some rogue code accidentally changes the value of event.which somewhere before the second if statement is evaluated. You will have a case where both blocks will be executed.

---

var addEditDelegateEvents = function(element, selector, url, idInUrl, idAttr, ajaxAttr) {

While there's nothing wrong with a sequence of arguments, it's often very hard to remember which one is which and the order they come in. Also, it's hard to set defaults. Suggesting you accept an object for configuration.

// Your module accepting options and merging it to defaults.
var defaults = {};

var addEditDelegateEvents = function(options){
  var mergedOptions = Object.assign({}, defaults, options);

}

// Usage
editable.addEditDelegateEvents({
  element: $("p.editable_sheet_date_end"),
  selector: "span",
  url: "/api/v1/sheets/ID/update",
  idInUrl: "ID",
  idAttr: "sheet_id",
  ajaxAttr: "end"
});

---

element.on("click", selector, function(event) {
  ...
  editElement.on("keypress", function(event) {

Nested event handlers can be hard to debug. It requires you to perform one action before you can test the other. What I suggest is to use event delegation for this. This way, you avoid nesting handlers, allowing you to attach handlers in advance, even when the element isn't present yet.

element.on('click', selector, function(event){
  // Create edit element with class "aClassOnEditElement"
});

// Listen for our element from an ancestor element
ancestorOfEditElement.on('keypress', '.aClassOnEditElement', function(){
  // Handle edit element event.
});

---

var DELAY = 300,
    clicks = 0,
    timer = null;

Major nitpick, but I suggest you do a var per variable. One issue you will have with this approach is during refactoring. You cannot easily re-arrange variables easily without having to re-add var, adjust commas and semi-colons.

Try putting timer as the first variable in the group with just cut-and-paste, you'll get what I mean.