1
\$\begingroup\$

The following code makes a div tag editable and handles the save and cancel buttons. But there is a lot of duplication. How could I remove the duplication on this?

var originalTexts = {};
$("div.edit-panel").find("span").click(function() {
    var panel = $(this).closest('div.simple-panel');
    var text  = panel.find(".info");
    var id = $(this).parent().attr("id");
    panel.find(".edit-panel").addClass('editable');
    text.addClass("content-editable").prop('contenteditable', 'true');
    originalTexts[id] = text.text();
})

$("div.edit-panel").find(".cancel-editable").click(function() {
    var panel = $(this).closest('div.simple-panel');
    var text  = panel.find(".info");
    var id    = $(this).parent().attr("id");
    text.text(originalTexts[id]);
    panel.find(".edit-panel").removeClass('editable');
    text.removeClass("content-editable").prop('contenteditable', 'false');
})

$("div.edit-panel").find(".save-editable").click(function() {
    var panel = $(this).closest('div.simple-panel');
    var text  = panel.find(".info");
    var currentText = text.text();
    text.text(currentText);
    panel.find(".edit-panel").removeClass('editable');
    text.removeClass("content-editable").prop('contenteditable', 'false');
})
\$\endgroup\$
1
\$\begingroup\$

How about using just one declaration and an if block? You remove the top three lines by this for each function -

assuming buttons are <button>

var originalTexts = {};
$("div.edit-panel").find("button,span").click(function(){
    var panel = $(this).closest('div.simple-panel');
    var text  = panel.find(".info");
    var id = $(this).parent().attr("id");
    var currentText = text.text();

    if ($(this).is("span")){
        panel.find(".edit-panel").addClass('editable');
        text.addClass("content-editable").prop('contenteditable', 'true');
        originalTexts[id] = text.text();
    }else if ($(this).hasClass("cancel-editable")){
        text.text(originalTexts[id]);
        panel.find(".edit-panel").removeClass('editable');
        text.removeClass("content-editable").prop('contenteditable', 'false');
    }else if ($(this).hasClass("save-editable")){
        text.text(currentText);
        panel.find(".edit-panel").removeClass('editable');
        text.removeClass("content-editable").prop('contenteditable', 'false');
    }
}); 

You can reduce more codes if you can store the id and other information inside the data attributes of each button. But I don't know the html, so could not suggest on that.

\$\endgroup\$
  • \$\begingroup\$ Very nice approach. What I did is putting all the code inside $("div.edit-panel").each(function()... and saving that three lines of code too \$\endgroup\$ – Pietro Coelho Jul 11 '14 at 23:30
1
\$\begingroup\$

It is always possible to avoid duplicate code but we need to think deep inside of actual requirements. ActiveHigh did good job but we can improve readability and remove duplicate code:

var originalTexts = {};
$("div.edit-panel").find("button,span").click(function(){
  var $currentTarget = $(this);
  var $panel = $currentTarget.closest('div.simple-panel');
  var $textElement  = $panel.find(".info");
  var id = $currentTarget.parent().attr("id");
  var currentText = $textElement.text();
  var contenteditable = false;

  if ($currentTarget.is("span")){
    contenteditable = true;
    originalTexts[id] = $textElement.text();
  }else if ($currentTarget.hasClass("cancel-editable")){
    $textElement.text(originalTexts[id]);
  }else if ($currentTarget.hasClass("save-editable")){
    $textElement.text(currentText);
  }

  var handelClass = (contenteditable?'add':'remove') +'Class';
  $panel.find(".edit-panel")[handelClass]('editable');
  $textElement[handelClass]("content-editable").prop('contenteditable', contenteditable);
});

Kindly consider following points when You use jQuery lib in your JavaScript:

1 - Naming convention for jQuery elements:

var $currentTarget = ...;
var $panel = ...;

2 - Cache DOM element for further usage, it improve performance.

var $currentTarget = $(this);

Happy JavaScript coding :)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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