Click inside Loop issue - Remove dynamically created elements

Viewed 41

The following code takes all the links onto the page that contains "https" and not "google.com" and turns them into iFrames. While that works, the close button that each is iFrame is supposed to be paired with does not work. When you click close, it only closes the last iFrame element on the page. I prefer to be able to do this in vanilla JavaScript, as opposed to jQuery.

total = []
var links = document.querySelectorAll("a");
for (var i = 0; i < links.length; i++) {
  var link = links[i];
  if (link.href.indexOf("https") != -1 && 
 link.href.indexOf("google.com") == -1) {
    var hey = (links[i].href);
    console.log(link.href);
    total.push(links[i].href);
    var iframe = document.createElement('iframe');
    iframe.src = hey;
    document.body.appendChild(iframe);
    var close = document.createElement('button');
    document.body.appendChild(close);
    close.innerHTML = "close";
    close.addEventListener('click',function(){
      console.log("click");
      document.body.removeChild(iframe);
      document.body.removeChild(close);
    })
  }
}
4 Answers

JS is not my primary language, but try this:

total = []
var links = document.querySelectorAll("a");
for (var i = 0; i < links.length; i++) {
    var link = links[i];
    if (link.href.indexOf("https") != -1 && link.href.indexOf("google.com") == -1) {
        var hey = (links[i].href);
        console.log(link.href);
        total.push(links[i].href);
        var iframe = document.createElement('iframe');
        iframe.src = hey;
        document.body.appendChild(iframe);
        var close = document.createElement('button');
        document.body.appendChild(close);
        close.innerHTML = "close";
        close.addEventListener('click',function(){
            console.log("click");
            document.body.removeChild(iframe);
            document.body.removeChild(close);
            document.body.removeChild(document.getElementById("iframe"));
        });
    }
}

It's a Variable Hoisting issue
where the var is hoisted to the closest scope, in your case it's window (since you don't have any other parent function wrapper), and reassigned/overridden again and again inside the loop - always leading to the last iterated element.

Quickfix:
var iframe and var close should be defined as const to remain inside the scope of that for loop body:

var links = document.querySelectorAll("a");
for (var i = 0; i < links.length; i++) {
  var link = links[i];
  if (link.href.indexOf("https") != -1 && link.href.indexOf("google.com") == -1) {
    const iframe = document.createElement('iframe');  // Quickfix
    iframe.src = links[i].href;
    iframe.id = Math.random();
    document.body.appendChild(iframe);
    const close = document.createElement('button');   // Quickfix
    document.body.appendChild(close);
    close.innerHTML = "close " + link.href;
    close.addEventListener('click', function() {
      document.body.removeChild(iframe);
      document.body.removeChild(close);
    })
  }
}
<a href="https://wikipedia.com"></a>
<a href="https://placekitten.com"></a>

The proper way

not only it's a bad habit to use var nowadays, it's also a bad practice to assign Event handlers inside a for loop. So here's a remake which removed completely the var keyword, uses some nifty reusable DOM utility functions, and at last — the NodeList.prototype.forEach() method:

// DOM utility functions:

const EL    = (sel, par)  => (par || document).querySelector(sel);
const ELS   = (sel, par)  => (par || document).querySelectorAll(sel);
const ELNew = (tag, prop) => Object.assign(document.createElement(tag), prop);

// Task:
// Convert all http/s anchors to iframes with a 
// button "Delete", wrapped inside a .figure DIV Element

ELS("a").forEach(EL_anchor => {

  const href = EL_anchor.href;
  
  if (/^https?:\/\/(?:(?:www\.)?google.com)/.test(href)) return;

  const EL_figure = ELNew("div", {className: "figure"});
  const EL_iframe = ELNew("iframe", {src: EL_anchor.href});
  const EL_delete = ELNew("button", {type: "button", textContent: "Delete", onclick() {EL_figure.remove();}});

  EL_figure.append(EL_iframe, EL_delete);
  EL("body").append(EL_figure);
});
<a href="https://wikipedia.com"></a>
<a href="https://placekitten.com"></a>

See the above's RegExp Example and desctription on Regex101.com

for (var i = 0; i < links.length; i++)

Change var to let.

OR

Use forEach instead of for loop.

The reason of this is scoping. var is function-scoped. Because above loop runs inside one function, var remains common for all loop iteration. On the other side, let or const are block-scoped. Because for loop creates individual blocks, each of the blocks created by the loop will work with an individual variable. forEach also creates individual scope. All variables will have values independent from each scope

If I understand correctly you want to create an <iframe> based on the href of pre-existing <a>s. You also want a <button> for each <iframe> that closes it.

In the example below are two functions:

  • linksToIframes(urlFrag)

    • Creates a box to put <iframe>s and <button>s into. Throwing them on <body> is messy:

      document.body.insertAdjacentHTML('afterBegin', `<fieldset></fieldset>`);
      const box = document.querySelector('fieldset');
      
    • Given a fragment of a url, it will collect all <a> into a HTMLCollection:

      const links = document.links
      
    • Convert HTMLCollection into an array and iterate through it finding any matches of href and urlFrag:

       [...links].forEach(link => {
         if (link.href.includes(urlFrag)) {...
      
    • Any match create the <frame> and <button> in the <fieldset> (the box):

         let iF = document.createElement('iframe');
         iF.src = link.href;
         box.appendChild(iF);
         ...
      
  • closeIframe(event)

    • An event handler that enables any <button> to remove the <frame> before it and itself:

      const clicked = e.target; // This is the tag user actually clicked
      if (clicked.matches('button')) { /* <button> is the only 
                                      tag that's accepted (that's 
                                      Event Delegation) */
       clicked.previousElementSibling.remove();
       clicked.remove();
       ...
      

Because of event bubbling we can bind the event handler on the parent element of all of the <button>s and then delegate how they react to a click. That's far better than an event handler on each <button>.

If you stick to your OP code, .previousElementSibling.remove(); applied to each <button> and .remove() to itself should fix it.

const linksToIframes = urlFrag => {
  document.body.insertAdjacentHTML('afterBegin', `<fieldset></fieldset>`);
  const box = document.querySelector('fieldset');
  const links = document.links;
  [...links].forEach(link => {
    if (link.href.includes(urlFrag)) {
      let iF = document.createElement('iframe');
      iF.src = link.href;
      box.appendChild(iF);
      let btn = document.createElement('button');
      btn.textContent = 'Close';
      box.appendChild(btn);
    }
  });
};
const closeIframe = e => {
  const clicked = e.target;
  if (clicked.matches('button')) {
    clicked.previousElementSibling.remove();
    clicked.remove();
  }
}

linksToIframes('https://example.com');
document.querySelector('fieldset').onclick = closeIframe;
a,
iframe,
button {
  display: block
}

iframe {
  width: 95%;
  max-height: 50px;
}
<a href='https://example.com'>EXAMPLE</a>
<a href='https://stackoverflow'>SO</a>
<a href='https://example.com'>EXAMPLE</a>
<a href='https://example.com'>EXAMPLE</a>

Related