-2

I have this piece of code and i can't prevent default link event all the time. It's working like 90% of the times, but the rest 10% my code doesn't run, get no error message and put a # on the end of my url. I have # on my href on my a tag.

I use .on() because sometimes the element is loaded after the document is ready. And i use .off() because before i used it, sometimes triggered more than once when i clicked on it. The strange part is sometimes. I don't get it how can a code sometimes work and sometimes dont. Shouldn't be the same result all the time?

Sorry for the lot of comments.

Here is my full jquery code:

//Handle box opening and closing
var boxcontent_width = $(".box-content").css("width");
$(".boxarrow").addClass("done");

$(document).on("click",".boxarrow", function(event){
    var serial = $(this).parent().parent().attr("serial");
    var clink = $(document).find("[comment-link='"+serial+"']");
    if(clink.hasClass("open")){
        clink.click();
    }

    if($(this).hasClass("done")){
        $(this).removeClass("done");
        $(document).find("#comment-box"+$(this).parent().parent().attr("serial")).slideUp(250);
        if($(this).parent().parent().find(".box-content").is(":hidden")){
            $(this).attr("src","source/up_arrow.png");
        }else{
            $(this).attr("src","source/down_arrow.png");
        }
        $(this).parent().parent().find(".box-content").slideToggle(500,function(){
            $(this).parent().find("div img").addClass("done");
        });            

    }
    return false;

}); 
//Handle votes
$(document).on("click","#positive-button, #negative-button", function(event){
    var serial = $(this).parents(".tutorial-box").attr("serial");
    var vote = 0;
    if($(this).attr("id") == "positive-button"){
        vote = 1;   
    }

    $.post("vote.php",{
        php_tutorial_id: serial,
        php_vote_value: vote
    },function(data){
        if(data.localeCompare("error") && data){
            if(data > 9999)data = 9999;
            if(data < -9999)data = -9999;
            if(data > 0){
                data = "+"+data;
            }
            $("#tutorial-vote"+serial+" p").fadeOut(250, function(){
                $("#tutorial-vote"+serial+" p").html(data).fadeIn(250);
            });
        }
    });
    return false;
});

//Handle comment opening and closing
$(document).on("click",".showcomments", function(event){
    if($(this).hasClass("open")){
        $(this).html("<p>Show comments</p>");
        $(this).removeClass("open");
    }else{
        $(this).html("<p>Hide comments</p>");
        $(this).addClass("open");
    }
    var clink = $(this).attr("comment-link");
    var is_open = $(this).hasClass("open");
    $(this).parents().find("#"+clink).slideToggle(500);
    return false;
});

//Handle add comment button
$(document).on("click",".comment-button", function(event){
    var serial = $(this).attr("serial");
    serial = "#comment-box"+serial;
    $(document).find(".comment-box").not(serial).slideUp(250);
    $(document).find(serial).slideToggle(250);
    return false;   
});
$(document).on("click",".comment-box-submit", function(event){  
    var tutorial_id = $(this).parent().attr("serial");
    var comment_text = $(this).parent().find(".comment-box-area");
    var serial = $(this).parent().attr("serial");
    $.post("send_comment.php",{
        php_tutorial_id: tutorial_id,
        php_comment_text: comment_text.val()
    },function(data){
        $(document).find("#comment-response"+serial).fadeOut(500, function(){
            if(data){
                $(document).find("#comment-response"+serial).html(data);
                $(document).find("#comment-response"+serial).fadeIn(500);
            }else{
                $(document).find("#comment-response"+serial).html("");
                $.post("reflesh_comments.php",{
                    php_tutorial_id: tutorial_id
                },function(data){
                    if($(document).find("#"+tutorial_id).is(":hidden")){
                        $(document).find("#"+tutorial_id).html(data);
                    }else{
                        $(document).find("#"+tutorial_id).fadeOut(500, function(){
                            $(document).find("#"+tutorial_id).html(data);
                            $(document).find("#"+tutorial_id).fadeIn(500);  
                        });

                    }

                });
                $(document).find("#comment-response"+serial).html("");
                comment_text.val("");
            }   
        });     
    }); 
    return false;
});
$(document).on("click",".comment-remove", function(event){  
    var serial = $(this).attr("serial");
    var tutorial_serial = $(this).parent().parent().parent().attr("id");
    $("#overlay-box").load("overlay_boxes/remove_comment.php", { php_serial: serial, php_tutorial_serial: tutorial_serial }, function(){
        $("#overlay-box").fadeIn(250);
        $("#overlay").fadeIn(250);

    });
    return false;
});


$(document).on("click",".comment-reply", function(event){
    var serial = $(this).parent().parent().attr("serial");
    var owner = $(this).parent().parent().find(".comment-owner").html();
    var owner_id = $(this).attr("serial");
    $(document).find("#comment-box"+serial).find(".comment-response").hide();
    $(document).find("#comment-box"+serial).slideDown(250); 
    $(document).find("#comment-box"+serial).find(".comment-box-area").val("@"+owner+": ");
    $(document).find("#comment-box"+serial).find(".comment-box-area").focus();
    return false;
});

$(document).on("click",".comment-report", function(event){  
    var serial = $(this).attr("serial");
    var tutorial_serial = $(this).parent().parent().parent().attr("id");
    $("#overlay-box").load("overlay_boxes/report_comment.php", { php_serial: serial }, function(){
        $("#overlay-box").fadeIn(250);
        $("#overlay").fadeIn(250);
    });
    return false;
});
Simonyi János
  • 96
  • 6
  • I think we're going to need a little more context to your code. Where is the jQuery you posted? Is it just in a script element? Or is it inside some other set of jQuery? – BurningLights Nov 20 '15 at 15:45
  • Simplest in my opinion would be to load the js code only once. And replace this `$(".comment-remove").off().on("click", function(event){ ` by `$(document).on("click", ".comment-remove", function(event){ ` – user5542121 Nov 20 '15 at 15:45
  • var serial = $(this).attr("serial"); Is bad practice, use a data tag instead. – ahervin Nov 20 '15 at 15:47
  • I see nothing in your code that would cause the problem described. – Kevin B Nov 20 '15 at 15:48
  • use javascript:void(0); inside 's href – Enjoyted Nov 20 '15 at 15:53
  • What part of the code do you need? This is activating an overlay to my site and load a php to my overlay-box placed middle of the screen. – Simonyi János Nov 20 '15 at 15:56
  • @SimonyiJános - please take a look at the answer I posted. I think it's the correct solution to your issue. – wahwahwah Nov 20 '15 at 16:45
  • It is. :) Thank you so much. Was suffering with this for hours. – Simonyi János Nov 20 '15 at 16:53
  • You're getting into a trend that can have a negative impact on your app performance later down the road; you're relying on event delegation way too much, or, your delegation base hasn't been picked very well. You already have 8 click events bound to the document, that means every time someone clicks on your page, jQuery will intercept that event and test it against each of those delegated events to see if it matches. This will increase the time between the user clicking and the action taking place, more so in mobile environments and older browsers. – Kevin B Nov 20 '15 at 17:08
  • How in the world do you know the event is being fired more than once? – RobertoNovelo Nov 20 '15 at 17:08
  • I still see nothing in your code that would cause the problem described. – Kevin B Nov 20 '15 at 17:09
  • As far as I can see, all of those should work just fine. If they are firing more than once, it's not caused by the code you have shown. – Robert McKee Nov 20 '15 at 17:10
  • A few of them is a slideToggle so i see the div slideup, down, up, down... – Simonyi János Nov 20 '15 at 17:11
  • My guess is you're including the whole script again on one (or more) of the pages you are ajaxing in, which will of course compound the delegation issue i mentioned above as well as cause event handlers to *appear* to run multiple times. – Kevin B Nov 20 '15 at 17:12
  • Do you have (or can you put) it up somewhere on a public URL? It would probably take less than a minute to find it. – Robert McKee Nov 20 '15 at 17:12
  • Yeh, that might be the problem. Did put the script to index.php instead, and now seems like working find. Did realized it when i wrote my last comment. > – Simonyi János Nov 20 '15 at 17:16
  • But there are a few value get in my functions. Should they work on elements have been loaded after my script loaded? – Simonyi János Nov 20 '15 at 17:17
  • If you are using the code in your question, yes. – Kevin B Nov 20 '15 at 17:17
  • They do, but i did think they're not, like the .click() things. – Simonyi János Nov 20 '15 at 17:17
  • The code will run at a point in time where the elements exist, so you'll be able to get the values from them. – Kevin B Nov 20 '15 at 17:18
  • Works fine. Stupid... So so stupid :c – Simonyi János Nov 20 '15 at 17:20
  • Use JSONs (or something alike) and separate your application from your services. It makes things easier I promise – RobertoNovelo Nov 20 '15 at 17:22

2 Answers2

0

You apparently have a multitude of issues, some of which we can't help with because you haven't included the relevant code.

  • You have your javascript code that attaches the event handler sometimes? executing more than once.
  • You are attaching the event to an element that may not exist at run time (are you executing the event handler attaching code when you insert it?)
  • You are removing all event handlers indiscriminately, possibly events that aren't your own.

The second and third problems are easy. Don't use 

$(".comment-remove").off().on("click", function(event){

use

$(document).on('click','.comment-remove',function(event){

I would also recommend not using the serial attribute, but use data-serial instead, and instead of var tutorial_serial = $(this).parent().parent().parent().attr("id"); use something like var tutorial_serial = $(this).closest('.tutorial').attr("id"); which is more maintainable.

Robert McKee
  • 21,305
  • 1
  • 43
  • 57
  • $(document) and $("body") are both fine? – Simonyi János Nov 20 '15 at 16:00
  • 1
    They are close to the same. `$("body")` attaches to the body element and requires a lookup, while `document` is the entire document and doesn't require a lookup. It's slightly faster, albeit it might actually bubble a little slower as it has to bubble up two more elements before it triggers. In most cases the difference isn't important. In general find an element that is static and closest container to where the stuff will go and use that. I tend to use `document` for any global stuff over body, but that's my preference. – Robert McKee Nov 20 '15 at 16:03
  • I guess it doesn’t make much difference. I tend to use body because that’s what is in the official jQuery API documentation. ¯\_(ツ)_/¯ – RobertoNovelo Nov 20 '15 at 16:26
  • The official documentation talks about both, but they only have an example of attaching to body. `By picking an element that is guaranteed to be present at the time the delegated event handler is attached, you can use delegated events to avoid the need to frequently attach and remove event handlers. This element could be the container element of a view in a Model-View-Controller design, for example, or document if the event handler wants to monitor all bubbling events in the document.` It really doesn't make much difference. – Robert McKee Nov 20 '15 at 16:35
  • The misconception is that a lot of people recommend against using `document` and to instead use something closer to the element they are binding to so that less elements on the page will run the logic that tests to see if the target of the event matches the delegate target, so... they lazily use `"body"`, which of course is no better than `document`. – Kevin B Nov 20 '15 at 16:49
  • A great example would be a table with dynamically generated rows. It would be far better to use the table as the base rather than document so that clicks in other areas of the page aren't compared against the delegate target selector, thus resulting in less code running. In most cases this won't impact performance, but it can if you're using an event that gets triggered often, such as keyboard events or scroll events, or if you have a bajillion delegated events with a base of document. (every event would have to go through all matching delegated events to see if the target matches delegate.) – Kevin B Nov 20 '15 at 16:57
  • @KevinB Yes. I mentioned that in my first comment, but it's good to reiterated it for others. You should find the closest container that is static to bind to. – Robert McKee Nov 20 '15 at 17:04
  • I think this conversation belongs to some other post though :P – RobertoNovelo Nov 20 '15 at 17:10
-1

Remove the href property on your anchor to prevent it from adding a # at the end of your url:

<a href="#" serial="'.$data_id.'" class="comment-remove" '.$hidden_remove.' >Remove</a>

And I would suggest using:

$("body").on("click",".comment-remove",function(){...})

To prevent the event firing more than once, since you are setting the listener on the body once, instead of on, which could or could not be assigned to your ID as desired, depending on wether the element exists in the DOM when that line is ran.

RobertoNovelo
  • 3,347
  • 3
  • 21
  • 32
  • Not sure why this is getting modded down as the second part is the correct answer. – Robert McKee Nov 20 '15 at 15:52
  • I tried this version as well. I rewrite it now to it. But the first part. If i delete # it doesn't put it to my url(didn't do it every click), but still my code doesn't run and nothing happens. But it's like 95% of the time working correctly. I'm just too lost :D – Simonyi János Nov 20 '15 at 15:55
  • Actually, my problem was missexplained. The double triggering was on a picture click event and the # was both picture and link. My bad. But now seems like working fine. Sorry and thank you :) – Simonyi János Nov 20 '15 at 16:20
  • No, the image click event still triggers twice. I don't get it. – Simonyi János Nov 20 '15 at 16:23
  • What class does your image have? – RobertoNovelo Nov 20 '15 at 16:25
  • Nah, was wrong. Still triggers it multiple times. Like working for a while then the next test it's toggles 4 times. Even checked if my mouse is bad. – Simonyi János Nov 20 '15 at 16:41
  • $("body").on("click",".comment-remove",function(){...}) *should not* be called within the function you load more elements on. It should be outside any other function. My best guess is that you are running that line more than once in your code. – RobertoNovelo Nov 20 '15 at 16:46