0

I am trying to delete an element in a ul (list item) like so using only javascript. since I am dynamically generating the html, I am trying to set the onclick as I create it below. However when I try to click (see fiddle) elements are not getting deleted. relevant functions below:

var appendHTML = function (el) {
    var list = document.getElementById("events");
    for (var i = 0; i < filteredDates.length; i++) {
        var listItem = filteredDates[i].name;
        listItem.className = "event-list";
        listItem.setAttribute("onclick", "deleteEvent()");
        list.appendChild(listItem);
    }
};


var deleteEvent = function () {
    var listItem = this.parentNode;
    var ul = listItem.parentNode;
    //Remove the parent list item from the ul
    ul.removeChild(listItem);
    return false;
};

https://jsfiddle.net/youngfreezy/7jLswj9b/5/

prompteus
  • 1,051
  • 11
  • 26
devdropper87
  • 4,025
  • 11
  • 44
  • 70
  • The code you posted here and the one in your fiddle don't match at all. How do you expect us to help you if you have two different versions of your code and we don't know which one to look at. – Felix Kling Nov 18 '15 at 22:14
  • Even the original version is different from what you posted here. I guess https://jsfiddle.net/youngfreezy/7jLswj9b/1/ is the one that you posted here. – Felix Kling Nov 18 '15 at 22:16

3 Answers3

5

Your first problem was that you had the "deleteEvent" in your html, jsFiddle doesn't like it.

second problem was that you took the wrong element in the deleteEvent so the parent was incorrect as well.

See updated fiddle: https://jsfiddle.net/7jLswj9b/7/

    var deleteEvent = function (ev) {
      var listItem = ev.target;
      var ul = listItem.parentNode;
      //Remove the parent list item from the ul
      ul.removeChild(listItem);
        return false;
    }

 var list = document.getElementById("events");
list.addEventListener("click",deleteEvent,false);
Saar
  • 2,276
  • 1
  • 16
  • 14
  • *"jsFiddle doesn't like it."* It's not that a tool can have feelings. You could at least explain the issues properly. – Felix Kling Nov 18 '15 at 22:18
  • @FelixKling , inline click events are valid and would have worked outside of jsfiddle, although they are not recommended. – Saar Nov 18 '15 at 22:29
  • It also works inside jsFiddle if setup correctly. Whether or not it's recommended is a different issue. – Felix Kling Nov 18 '15 at 22:30
  • 1
    I didn't know there is that option to set it "correctly", but criticizing the answer due to a specific tool issue is a different issue as well... – Saar Nov 18 '15 at 22:32
  • I'm not criticizing your answer for jsFiddle issues, I'm criticizing it for not providing any explanation. – Felix Kling Nov 18 '15 at 22:34
1

If you use the onclick attribute to set event code as a string, the this context gets lost.

The best way to register event handlers is via addEventListener:

listItem.addEventListener("click", deleteEvent);

As a fallback, set your event code as a function instead:

listItem.onclick = deleteEvent;

This is discouraged as you can only set 1 event handler per event this way.


If you really really have to set the code Inline, pass the value of this via an argument:

<ul id="events" onclick="deleteEvent(this)">...</ul>

function deleteEvent(element) {
  // ...
}
Leon Adler
  • 2,993
  • 1
  • 29
  • 42
1

There are a couple of things wrong with your jsFiddle and your code.

Scope of the event handler

You are using an inline event handler. Inline event handlers are executed in global scope. However, because the fiddle by default wraps your JavaScript code inside a function, deleteEvent is not global. The console shows the error:

Uncaught TypeError: Cannot read property 'appendChild' of null

There are three ways to solve this:

this inside the event handler

Because you are calling the function as deleteEvent(), this won't refer to the DOM element that triggered the event. It will refer to window, the global object, which doesn't have a parentNode property.

If you use a different way to bind the handler, as suggested above, then that problem might solve itself. Otherwise, you have to call deleteEvent in such a way that this refers to the element:

onclick="deleteEvent.call(this)"

Of course you could also change the definition of deleteEvent to accept the element as argument instead.

However, in your fiddle you actually attached the event handler to the <ul> element, not to each <li> element. In that case you have to pass the event object to function instead and get the original target from it:

onclick="deleteEvent(event)"

and

var deleteEvent = function (event) {
  //Remove the parent list item from the ul
  event.target.parentNode.removeChild(event);
}

DEMO

Wrong node reference

It seems you are deleting the wrong node in deleteEvent. listItem will actually be the <ul> element, because that's the parent of the clicked element. Consequently, ul will be the parent of the <ul> element.

Since this already refers to the <li> element, there is need for listItem here, and ul should be this.parentNode. In shorter form:

var deleteEvent = function () {
  //Remove the parent list item from the ul
  this.parentNode.removeChild(this);
}

return false has no effect

return false is often used to prevent the default action of the event. However, clicking on a li element has no default action. Furthermore, the value has to be return from the event handler. deleteEvent is not your event handler, it is called by your event handler. So if you want to return its return value, you have to do exactly that:

listItem.setAttribute("onclick", "return deleteEvent()");

But again, you should prefer a different of binding the event handler.

Invalid node creation

In the code in your question you are trying to call setAttribute on an arbitrary object and try to append an arbitrary object to a DOM element. That won't work. You have to use document.createElement to create a new element:

var li = document.createElement('li');

It seems you are doing this correctly in your fiddle.

Community
  • 1
  • 1
Felix Kling
  • 795,719
  • 175
  • 1,089
  • 1,143
  • very interesting solution. that deletes the entire ul and not individual "li"s though! – devdropper87 Nov 18 '15 at 21:54
  • @devdropper87: Well, that's the logic you have in `deleteEvent`. So I guess the third issue is that your logic in there is incorrect. – Felix Kling Nov 18 '15 at 21:55
  • Also the code you posted here is different than the code in your fiddle. Here you are binding the handler to each `li` element, in your fiddle you attached the handler to the `
      ` element.
    – Felix Kling Nov 18 '15 at 22:12
  • @devdropper87 in my answer above I have corrected your code so you can see what was wrong – Saar Nov 18 '15 at 22:17