How do I only call my event handler once and not every time i click an item?

Question

Particularly I'm concerned with adding to many event handlers each time an item is clicked.

$(".container-items").on('click', '.item', function(event){

    //show modal
    $("#modal-background, #modal").show();

    //get the current item contents and store it in a variable
    var itemContent = $(event.currentTarget).text();

    //populate textarea content with current "itemContent"
    $("#editableText").val(itemContent);

    //when user clicks cancel
    $(".cancel").on("click", function(){
        $("#editableText").val("");
        $("#modal-background, #modal").hide();
        $(".cancel").off("click");
        $(".update").off("click");
    });

    //when user clicks update
    $(".update").on("click", function() {
        var newItemContent = $("#editableText").val();
        $(event.currentTarget).text(newItemContent);
        $("#modal-background, #modal").hide();
        $(".update").off("click");
    });

});

Here's my jsfiddle: http://jsfiddle.net/3aa4T/

Here's my HTML:

<div class="add-item-wrapper">
    <div class="textbox">
        <input id="item-value" type="text" />
    </div>
    <div class="add-item-btn">
        <button>Add Item</button>
    </div>
</div>
<div class="error">
    <p>Sorry you have to add something to the list.</p>
</div>
<ul class="container-items">

</ul>

<button id="delete-all">Delete All Items</button>

<div id="modal-background"></div>
<div id="modal">
    <div class="container-wrapper">
        <div class="input-text">
            <textarea id="editableText" rows="4" cols="20"></textarea>
        </div>

        <button class="update">Update</button>
        <button class="cancel">Cancel</button>
    </div>

</div>

Here's my javascript:

$( document ).ready(function() {

    var value = "";

    //key up on keyboard event handler
    $("#item-value").keyup(function( event ){

        value = $("#item-value").val();

        //if user pressed enter
        if(event.which === 13){
            if( !$(this).val() ){
                $(".error").fadeIn("slow").fadeOut("slow");
            } else {
                $(".container-items").append("<li class='item'>" + value + "</li>");
                $("#item-value").val("");
                value = ""; 
            }
        }
    });

    //Submit item using "Add Item" button
    $(".add-item-btn").click(function(){

        value = $("#item-value").val();

        //check to see if empty string
        if ( value === "" ){
            $(".error").fadeIn("slow").fadeOut("slow");
        } else {
            $(".container-items").append("<li class='item'>" + value + "</li>");
            $("#item-value").val("");
            value = ""; 
        }

    });     

    //Empty all items in "container-items" class
    $("#delete-all").click( function(event){
        $(".container-items").empty();
    });

    //click an item
    $(".container-items").on('click', '.item', function(event){

        //show modal
        $("#modal-background, #modal").show();

        //get the current item contents and store it in a variable
        var itemContent = $(event.currentTarget).text();

        //populate textarea content with current "itemContent"
        $("#editableText").val(itemContent);

        //when user clicks cancel
        $(".cancel").on("click", function(){
            $("#editableText").val("");
            $("#modal-background, #modal").hide();
            $(".cancel").off("click");
            $(".update").off("click");
        });

        //when user clicks update
        $(".update").on("click", function() {
            var newItemContent = $("#editableText").val();
            $(event.currentTarget).text(newItemContent);
            $("#modal-background, #modal").hide();
            $(".update").off("click");
        });

    });

});

As your question is written, it is off topic for SO since it is code review. Can you rewrite it to point at specific issues you have? — mplungjan, Nov 10 '13 at 07:43
You need to save items you already accessed and use var and $(this) a lot more like `$("#item-value").keyup(function( event ){ var value = $(this).val(); ... $(this).empty()` — mplungjan, Nov 10 '13 at 07:45
@mplungjan thanks! Just updated the question. I want to make sure that I'm not adding event handlers each time I click an item. I should call .on() once, right? — rishtal, Nov 10 '13 at 07:56

Roko C. Buljan · Answer 1 · 2013-11-10T08:28:54.280

2

LIVE DEMO

_{Note, I changed also a bit of HTML and CSS, basically I added the modal into it's background container.}

$(function() {

    var $itemValue      = $('#item-value'),        // Cache your elements!
        $modal          = $("#modal-background"),
        $editable       = $("#editableText"),
        $itemsContainer = $(".container-items"),
        $selected;        // Will keep track of the active (selected) item

    $itemValue.keyup(function(e) {
        if(e.which===13) $(".add-item-btn").click();
    });

    $(".add-item-btn").click(function() {
        var value = $.trim( $itemValue.val() );
        if (!value){
            $(".error").fadeIn("slow").delay(800).fadeOut("slow");
        } else {
            $itemsContainer.append("<li class='item'>" + value + "</li>");
            $itemValue.val("");
        }
    });     

    $("#delete-all").click( function(e) {
        $itemsContainer.empty();
    });

    $itemsContainer.on('click', '.item', function(e) {
        $selected = $(this);     
        $modal.show();
        $editable.val( $selected.text() );
    });

    $(".cancel, .update").on("click", function(){
        $modal.hide();
        return this.className == 'update' ? // clicked '.update'?
          $selected.text($editable.val()) : // yes
          $editable.val("");                // no
    });

});

edited Nov 10 '13 at 08:28

answered Nov 10 '13 at 08:02

Roko C. Buljan

196,159
39
305
313

I understand the majority of your update but could you please help me understand what's wrong with: $(".container-items").on('click', '.item', function(event){ ...} that i wrote above.thanks! – rishtal Nov 10 '13 at 08:17
@rishtal oh, sorry, basically nothing cause you're using `.on()` and `.off()` methods. My bad. Tho I don't see any reason to use on / off unless you're storing that data immediately to server, which might prevent a fast-click... or something, messing your data. Any way. It's more readable, and don't add comments to code that is self-explanatory. – Roko C. Buljan Nov 10 '13 at 08:23
@rishtal also added a error delay, so the user has the time to read it ;) – Roko C. Buljan Nov 10 '13 at 08:26
@rishtal also added `var value = $.trim( $itemValue.val() );` to prevent `empty space` be validate as a valid input. – Roko C. Buljan Nov 10 '13 at 08:29
Thanks! I appreciate it. so general rule is not to nest event handlers? – rishtal Nov 10 '13 at 08:31
@rishtal basically, unless you have the specific exclusive need to add an event handler only after another event. I'm passionate about minification, (as you can see the ternary operator `?:` in action) but at the same time make the code fast, *globally* readable and clean. – Roko C. Buljan Nov 10 '13 at 08:36

score 0 · Answer 2 · answered Nov 10 '13 at 07:37

I would make the "add" section a form... and just use the submission of the form instead of the .keyup and button .click handlers. I made a jsfiddle - http://jsfiddle.net/3aa4T/1/ with the key lines being

$("#add-item-form").submit(function(e){
    e.preventDefault();

How do I only call my event handler once and not every time i click an item?

2 Answers2