10

My html code is as follows:

<h4 class="milestonehead" style="cursor: pointer;" onclick="editFun('35');">
     Some Header
     <img src="/images/delete.gif" style="float: right;" onclick="deletefun('35');">
</h4>

There are two functions in <h4> if user click on header i.e <h4> then I need to open a popup with edit form. If user click on delete image/icon then I need to execute a delete function.

Both functions editFun and deletefun execute ajax calls. In my case if the user clicks on delete icon then first it calls editFun function and then it is calling deleteFun. How can I call the appropriate function for the event.

Arslan Ali
  • 17,418
  • 8
  • 58
  • 76
Swap L
  • 698
  • 3
  • 12
  • 27

5 Answers5

5

If you're using jQuery, get rid of the inline JS and CSS and move them into their own files:

CSS:

.milestonehead { cursor: pointer; }
.image { float: right; }

HTML

<h4 class="milestonehead" data-id="35">
  Some Header
  <img class="image" src="/images/delete.gif">
</h4>

JS

$(document).on('click', '.milestonehead', function () {
  var id = $(this).data('id');
  editFun(id);
});

In this case you can just use the data id on the parent node. Data attributes are by far the best way to store data on your HTML elements.

$(document).on('click', '.image', function (e) {
  e.stopPropagation();
  var id = $(this).parent().data('id');
  deleteFun(id);
});
Andy
  • 61,948
  • 13
  • 68
  • 95
  • my markup is added dynamically via ajax call so do i need to use .live or .bind event insted of .on ? – Swap L Jan 30 '14 at 14:31
  • Nope. `on` replaced `live` in jQuery. This is the best way of dealing with [event delegation](http://api.jquery.com/on/#direct-and-delegated-events). – Andy Jan 30 '14 at 14:39
  • If you're adding to the DOM after the page has loaded you need to use event delegation like I've mentioned above. `$('.image').click(function ()...` will not work. – Andy Jan 30 '14 at 15:01
  • yes. in answer of Robert Koritnik using .click event. Now can you tell which is better to refer with .CLASSNAME or [ATTRIBUT] as mention in Robert Koritnik answer. – Swap L Jan 30 '14 at 15:04
  • It depends if clicking on a `.milestonehead` class will always call `editFun`. If so, perhaps stick with classes, but I do like Robert's answer. It's entirely up to you. What do _you_ prefer? – Andy Jan 30 '14 at 15:11
3

Your problem = click propagation

Events in browser are being propagated along DOM element tree. You have to stop that in order for each call to individually execute. Your code also sets click handlers inline so I'd rather suggest you separate functionality from your markup.

Use jQuery

Instead of setting click handlers inline use jQuery:

$(".milestonehead img").click(function(evt) {
    evt.preventDefault();
    deletefun("35");
});

And similar for your H4. Mind that my jQuery selector may not be appropriate in your situation but you can always distinguish particular elements by IDs or CSS classes.

Separation of concerns

Externalizing your functionality to code only file (or at least code block in your page) makes your page much more maintainable.

Set event handlers in DOM Ready event:

$(function(){
    $(".milestonehead").click(function(evt) {
        evt.preventDefault();
        editfun("35");
    });

    $(".milestonehead img").click(function(evt) {
        evt.preventDefault();
        deletefun("35");
    });    
});

Improving markup

It seems that you have a list of some items (as you're doing something related to item 35) where each one is being represented by h4 and nested delete icon. Hence I'd rather change markup (and code) to do this:

<h4 class="milestonhead" data-edit-item="35">
    Some header
    <img src="/images/delete.gif" data-delete-item="35" />
</h4>

Style your elements using CSS file instead (separation of concerns again either in separate CSS file or style block on your page) and you can thank me for this later.

.milestonehead {
    cursor: pointer;
}

.milestonehead img {
    float: right;
}

Code to handle these items would then look like this:

$(function() {

    $("[data-edit-item]").click(function(evt){
        evt.preventDefault();
        editfun($(this).data("editItem"));
    });

    $("[data-delete-item]").click(function(evt){
        evt.preventDefault();
        deletefun($(this).data("deleteItem"));
    });

});

This creates two common click handlers for any element that would have appropriate data attributes (data-edit-item or data-delete-item). So your header may have these as well some "Edit" link too. Just set appropriate values to that particular data attribute.

Robert Koritnik
  • 103,639
  • 52
  • 277
  • 404
  • this markup is added dynamically so do i need to use live or bind method? – Swap L Jan 30 '14 at 14:27
  • 1
    $(document).on('click', '[data-edit-item]', function () { evt.preventDefault(); editfun($(this).data("edit-item")); }); is working fine i really appreciate your answer – Swap L Jan 30 '14 at 14:57
2

It's going to be much easier if you use jQuery to bind your event handlers. You'll need to make a modification to your HTML so that you can identify the ID (I assume the 35 is an ID, at any rate) for that particular call.

<h4 class="milestonehead" style="cursor: pointer;" data-id="35">
     Some Header
     <img src="/images/delete.gif" style="float: right;">
</h4>

Then the jQuery:

$('h4').on('click', function(e) {
    var id = $(this).attr('data-id');
    editFun(id);
}).find('img').on('click', function(e) {
    e.stopPropagation(); // this will prevent the click event being handled by the h4 as well
    var id = $(this).closest('h4').attr('data-id');
    deletefun(id);
});

EDIT: You might want to change the start of the code to $('h4.milestonehead') depending on whether or not all of the H4 elements that should have that functionality also have that class. If they do, make the change. If they don't, don't make the change (but probably add some class to make them distinguishable from other H4 elements).

Anthony Grist
  • 38,173
  • 8
  • 62
  • 76
  • with this you should get rid of the `onclick` attributes in the HTML, and you could use `.data('id')` instead of `.attr('data-id')` – Jamiec Jan 30 '14 at 13:49
  • @Jamiec Yeah, forgot to remove the `onclick` attributes; noticed that myself just now and removed them. I *could* use `.data()` but I chose not to, mostly because I don't want the implied type conversion that jQuery does when using it. That's probably not going to make a difference either way though. – Anthony Grist Jan 30 '14 at 13:50
1

First thing is to check what your target is. In your event you should be running

function(event) {
    //code here
}

You can use event.target to find out what element in the DOM you've actually hit. From here you can use this value to determine if you should run the function or not, for example if the element is the "img" then don't run editFun()!

For something this simple though, you may be better off putting event.stopPropagation(); in your deleteFun function, since it's a simple "either/or" kind of situation.

More info on this method can be found here: How to stop event bubbling on checkbox click

Community
  • 1
  • 1
niaccurshi
  • 1,265
  • 9
  • 9
  • This assumes that you follow Robert's advice and don't code your javascript inline, which is really just good practice. If you need to pass a value use a data-value="35" on your h4, and then use javascript to get that attribute. – niaccurshi Jan 30 '14 at 13:49
1

You forgot about event propagation. Use this:

<h4 class="milestonehead" style="cursor: pointer;" onclick="return editFun('35');">
     Some Header
     <img src="/images/delete.gif" style="float: right;" onclick="return deletefun('35');">
</h4>

editFun and deletefun must ends return false;

Alex
  • 11,115
  • 12
  • 51
  • 64