Using removeChild() properly

Question

I have a function that appends a "div" child into a parent node, then I need to delete this child using the removeChild() method, but it doesn't work.

This is my code:

function ColorCards()
        {for (i=0; i<numerocaselle; i++)
            {document.getElementsByClassName("MemoryCards")[i].style.border="none"
            var esempiocolore=document.createElement('div')
            esempiocolore.style="position: relative; height: 80px; width: 130px; background-image: url('img/backcard"+cartaesempio+".png'); background-size: cover;"
            document.getElementsByClassName("MemoryCards")[i].appendChild(esempiocolore)
            }
        }

        function CleanColorCards()
        {for (i=0; i<numerocaselle; i++)
            {document.getElementsByClassName("MemoryCards")[i].style.border="dashed 3px #02A494"
            document.getElementsByClassName("MemoryCards")[i].removeChild(document.getElementsByTagName("div"))
            }
        }

Does somebody have any suggestion on how to make it work?

There is not any HTML because I create all my using another JS function that assigns a class (MemoryCards) to these — Simone Pinto, May 05 '19 at 19:53
There will still be some DOM elements, which you can render the HTML of here, so we can understand the structure — ADyson, May 05 '19 at 19:58

trincot · Answer 1 · 2019-05-05T20:55:32.507

0

You are passing an NodeList to removeChild, while you should pass a single node. Secondly, document.getElementsByTagName("div") is going to also find elements that are not children of the parent you are trying to remove a child from.

So do it like this:

// Avoid repetition of code, and store the result in a variable:
var nodelist = document.getElementsByClassName("MemoryCards");
for (var i=0; i<numerocaselle; i++){
    const parent = nodelist[i];
    parent.style.border="dashed 3px #02A494";
    // Perform `getElementsByTagName` on the parent node only, and take first match:
    parent.removeChild(parent.getElementsByTagName("div")[0]);
}

Note that querySelector is designed for getting one node-result, so that last line can read:

    parent.removeChild(parent.querySelector("div"));

edited May 05 '19 at 20:55

answered May 05 '19 at 19:54

trincot

263,463
30
215
251

Why in the world do people believe that scanning the DOM for all matches, making a node list and then throwing that node list away just to use the first match is a good idea?[Just use `.querySelector()`](https://stackoverflow.com/questions/54952088/how-to-modify-style-to-html-elements-styled-externally-with-css-using-js/54952474#54952474). – Scott Marcus May 05 '19 at 20:14
@ScottMarcus, Sure, `querySelector` avoids creating a node list (added to answer). It is likely from the OP's code, that there is only one child `div` anyway, so the gain (relative to the cost of accessing the DOM) will be very limited. – trincot May 05 '19 at 20:57
It doesn't matter how many matches there are, it matters how big and deep the DOM is, so if you are scanning the entire DOM for just the first matching element, the extra work could be much more than limited. – Scott Marcus May 05 '19 at 21:11
That is true for the OP's code, which did not work anyway. But in my answer only the parent node is searched which suppsedly only has one child. – trincot May 05 '19 at 21:41
But, if the parent node will be completely searched. `.getElementsByTagName()` isn't going to stop at the first match. If the parent node is deep, you've got the same problem. – Scott Marcus May 05 '19 at 21:50

score 0 · Answer 2 · answered May 11 '19 at 22:00

Just a couple notes:

Using a for loop is unnecessary. Having a variable to hold the count of the length of .MemoryCards will leave room for errors. Instead, I recommend an Array function such as .forEach() to iterate through your elements.
The bulk of your element styles should be handled with classes in CSS. By doing this your function will be more concise and easier to manage.
And, to answer your question:
1. To remove all child nodes for each .MemoryCards, I would recommend using a loop and the node.removeChild() method as it will perform faster than setting node.innerHTML=''.
2. See the comments in this post as why this method would be best.

let cartaesempio = 10;
ColorCards = () =>
  Array.from(document.getElementsByClassName("MemoryCards"))
  .forEach(e => {
    e.classList.add('borderNone');
    let esempiocolore = document.createElement('div');
    esempiocolore.style.backgroundImage = `url('https://cdn.dribbble.com/users/285803/screenshots/1066705/dribbble.gif')`;
    e.appendChild(esempiocolore);
  });

CleanColorCards = () =>
  Array.from(document.getElementsByClassName("MemoryCards"))
  .forEach(e => {
    e.classList.add('boderDashed');
    while (e.firstChild) {
      e.removeChild(e.firstChild);
    }
  });

ColorCards();
CleanColorCards();

/* Children of the .MemoryCards nodes */

.MemoryCards div {
  position: relative;
  height: 80px;
  width: 130px;
  background-size: cover;
}

.borderNone {
  border: none;
}

.boderDashed {
  border: dashed 3px #02A494;
}

<div class='MemoryCards'></div>

Hope this helps,

score -1 · Answer 3 · answered May 05 '19 at 19:53

-1

getElementsByTagName returns node list(array). You will have to select a node. Maybe something like this would be useful for you: document.getElementsByTagName("div")[0]

https://developer.mozilla.org/en-US/docs/Web/API/Element/getElementsByTagName

answered May 05 '19 at 19:53

Cihangir Bozdogan

662
6
13

`.getElementsByTagName()` is not a good suggestion in 2019. Maybe in 2002. Also, it makes no sense to scan the entire DOM, make a node list and then throw away all but the first item found. Just use `.querySelector()` [Read this please](https://stackoverflow.com/questions/54952088/how-to-modify-style-to-html-elements-styled-externally-with-css-using-js/54952474#54952474). – Scott Marcus May 05 '19 at 20:12
Well I told him why his code breaks. You come up with a better practice. querySelector is not working on IE6 - IE7. May be he has to use getElementsByTagName – Cihangir Bozdogan May 07 '19 at 08:57
IE7 came out 13 years ago! There is no need to write code that is compatible for it anymore and hasn't been for many years. The only reason we still see people using `.getElementsByTagName()`, `.getElementsByClassName()`, `.getElementsByName()`, or inline event attributes like `onclick="..."` is simply because enough people aren't educating the new coders not to use these woefully out of date API's. No one *has* to use any of these in 2019. And, if by some strange edge case, someone does, you can be assured that they will state that in their question. – Scott Marcus May 07 '19 at 12:38
Great. Thanks Scott – Cihangir Bozdogan May 09 '19 at 10:50

score -1 · Answer 4 · answered May 05 '19 at 19:55

-1

just check your error ! document.getElementsByTagName("div") return an array of div that's basic js meaning you've to search more by ourself. Use a manual, like the one at w3schools or a book whatever :-)

answered May 05 '19 at 19:55

jean3xw

117
6

score -1 · Answer 5 · answered May 05 '19 at 21:01

-1

You are passing removeChild all the divs in the document. Try replacing this line:

document.getElementsByClassName("MemoryCards")[i].removeChild(document.getElementsByTagName("div"))

With:

var memoryCardsEl = document.getElementsByClassName("MemoryCards")[i];
memoryCardsEl.removeChild(memoryCardsEl.getElementsByTagName("div")[0]);

answered May 05 '19 at 21:01

benkol

350
1
12

what if there are some other tags and the divs I dont want to remove? – Mr.roland May 06 '19 at 01:40
@fengo Then you should use something else and not getElementsByTagName("div") – benkol May 06 '19 at 11:27

Using removeChild() properly

5 Answers5