0

All,

I have a 'credit module' (similar to credit system in games), which when a user performs an action, creates an inner div with the cost to be added or substracted so user can see what the cost of the last action was.

Problem: Everything works fine as long as the function is called once, if the user performs multiple actions quickly, the setTimeout functions (which are suppose to animate & then delete the cost div) donot get executed. It seems the second instance of the function resets the setTimeout function of the first.

(function()
{

$("#press").on("click", function(){creditCost(50)});

function creditCost(x)
{
var eParent = document.getElementById("creditModule");
// following code creates the div with the cost
eParent.innerHTML += '<div class="cCCost"><p class="cCostNo"></p></div>';
var aCostNo = document.getElementsByClassName("cCostNo");
var eLatestCost = aCostNo[aCostNo.length - 1];
// following line assigns variable to above created div '.cCCost'
var eCCost = eLatestCost.parentNode;
// cost being assigned
eLatestCost.innerHTML = x;
$(eCCost).animate ({"left":"-=50px", "opacity":"1"}, 250, "swing");
// following code needs review... not executing if action is performed multiple times quickly
setTimeout(function()
{
    $(eCCost).animate ({"left":"+=50px", "opacity":"0"}, 250, "swing", function ()
    {
        $(eCCost).remove();
    })  
}, 1000);
}

})();

jsfiddle, excuse the CSS

Kayote
  • 14,579
  • 25
  • 85
  • 144
  • Im trying to use as much js as possible as our web application is pretty heavy as it is & we are trying to rely on jquery only for onload events & bindings. – Kayote Aug 26 '12 at 04:57

3 Answers3

2
eParent.innerHTML += '<div class="cCCost"><p class="cCostNo"></p></div>';

is the bad line. This resets the innerHTML of your element, recreating the whole DOM and destroying the elements which were referenced in the previous invocations - letting their timeouts fail. See "innerHTML += ..." vs "appendChild(txtNode)" for details. Why don't you use jQuery when you have it available?

function creditCost(x) {
    var eParent = $("#creditModule");
    // Create a DOM node on the fly - without any innerHTML
    var eCCost = $('<div class="cCCost"><p class="cCostNo"></p></div>');

    eCCost.find("p").text(x); // don't set the HTML if you only want text

    eParent.append(eCCost); // don't throw over all the other children
    eCCost.animate ({"left":"-=50px", "opacity":"1"}, 250, "swing")
          .delay(1000) // of course the setTimeout would have worked as well
          .animate ({"left":"+=50px", "opacity":"0"}, 250, "swing", function() {
               eCCost.remove();
          });  
}
Community
  • 1
  • 1
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • If my understanding is correct, only the eParent part of DOM is affected. Im 'adding' to the existing struction of eParent (#creditModule div). – Kayote Aug 25 '12 at 16:21
  • No, he only does weird things with parentNodes etc. - his `eCCost` does point to the just created (outer) element, but he destroys it when resetting the innerHTML – Bergi Aug 25 '12 at 16:32
  • Ive spent well over an hour thinking about the explanation you've written and I still cannot wrap my head around this. As Im using '+=' to add the '#creditModule' div, how is it possible that the DOM is being reset? (Also, please see comment posted under my question above). – Kayote Aug 26 '12 at 04:56
  • 1
    I've added a link to an [explanation](http://stackoverflow.com/a/2305677/1048572) – Bergi Aug 26 '12 at 12:05
1

You are starting an animation and scheduling a timeout to work on DOM elements that will get modified in the middle of that operation if the user clicks quickly. You have two options for fixing this:

  1. Make the adding of new items upon a second click to be safe so that it doesn't mess up the previous animations.
  2. Stop the previous animations and clean them up before starting a new one.

You can implement either behavior with the following rewrite and simplification of your code. You control whether you get behavior #1 or #2 by whether you include the first line of code or not.

function creditCost(x) {
    // This first line of code is optional depending upon what you want to happen when the 
    // user clicks rapid fire.  With this line in place, any previous animations will
    // be stopped and their objects will be removed immediately
    // Without this line of code, previous objects will continue to animate and will then
    // clean remove themselves when the animation is done
    $("#creditModule .cCCost").stop(true, false).remove();

    // create HTML objects for cCCost
    var cCCost = $('<div class="cCCost"><p class="cCostNo">' + x + '</p></div>');
    // add these objects onto end of creditModule
    $("#creditModule").append(cCCost);

    cCCost
        .animate ({"left":"-=50px", "opacity":"1"}, 250, "swing")
        .delay(750)
        .animate({"left":"+=50px", "opacity":"0"}, 250, "swing", function () {
            cCCost.remove();
        });  
    }
})();

Note, I changed from setTimeout() to .delay() to make it easier to stop all future actions. If you stayed with setTimeout(), then you would need to save the timerID returned from that so that you could call clearTimeout(). Using .delay(), jQuery does this for us.

jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • no, I don't think his problem is with the animations. He tries to animate different elements. – Bergi Aug 25 '12 at 16:19
  • @Bergi - my initial explanation wasn't quite correct (I have since reworded), but I think the idea is right and the suggested code will work. – jfriend00 Aug 25 '12 at 16:27
  • That code does not work - in the first line you remove a element (the `eCCost` variable) that does not exist at that moment – Bergi Aug 25 '12 at 16:29
  • @Bergi - If the user is going fast, `eCCost` does exist at the time (because the previous animation and `setTimeout()` hasn't completed). If it doesn't exist, then it is a harmless operation as the `$(eCCost)` will simply be empty and the method calls on it will do nothing. So essentially what this code is doing is saying: "If the previous operation hasn't completed yet, then stop it and get rid of the `eCCost` object so we can start fresh with a new operation". – jfriend00 Aug 25 '12 at 16:31
  • Ah, you've already changed it to a class selector - what I meant was that the local variable would have been undefined – Bergi Aug 25 '12 at 16:35
  • Rewritten the core code to make it a lot simpler and easier to understand. – jfriend00 Aug 25 '12 at 16:51
  • @jfriend00, thanks for the explanation. I did do something similar (& a bit shorter I think). I inserted following line to remove previous instances but the resultant effect doesnt look nice: if (document.getElementsByClassName("cCCost").length > 0) {$(".cCCost").remove();} – Kayote Aug 26 '12 at 04:59
  • 1
    @Kayote, just so you know - in jQuery, you don't have to check if there are any elements before calling a method. You can just do `$(".cCCost").remove();`. If there are no `.cCCost` items, then it just does nothing. – jfriend00 Aug 26 '12 at 05:01
0

Updated code for anyone who might want to do with mostly javascript. Jsfiddle, excuse the CSS.

function creditCost(x)
{
    var eParent = document.getElementById("creditModule");
    var eCCost = document.createElement("div");
    var eCostNo = document.createElement("p");
    var sCostNoTxt = document.createTextNode(x);
    eCCost.setAttribute("class","cCCost");
    eCostNo.setAttribute("class","cCostNo");
    eCostNo.appendChild(sCostNoTxt);
    eCCost.appendChild(eCostNo);
    eParent.insertBefore(eCCost, document.getElementById("creditSystem").nextSibling);
    $(eCCost).animate ({"left":"-=50px", "opacity":"1"}, 250, "swing");
    setTimeout(function()
    {
        $(eCCost).animate ({"left":"+=50px", "opacity":"0"}, 250, "swing", function ()
        {
            $(eCCost).remove();
        })  
    }, 1000);
}
Kayote
  • 14,579
  • 25
  • 85
  • 144