2

I am trying to get the button below with addEventListener. How ever it returns null. The html is rendered from js using template string. So what I am trying to achieve is to addEventListener to delete button inside the template string.

// This is the template string
data.forEach(doc => {
        checkin = doc.data();
        card = `
        <div class="carousel-item fieldId" data-id="${doc.id}">
        <div class="col-12">
            <div class="card checkCard" style="margin: 0 auto;">
                <img src="${checkin.photo}" class="card-img-top" alt="...">
                <button type="submit" class="btn center delete">
                    <i class="material-icons" style="font-size: 1em;">delete_forever</i>
                </button>
                <div class="card-body">
                    <h5 class="card-title">${checkin.title}</h5>
                    <p class="card-text">${checkin.description}</p>
                </div>
            </div>
        </div>
        </div> 

        `;
        html += card;
    });

    checkinList.innerHTML = html;


//This is the delete button

const deleteContent = document.querySelector('.delete');
deleteContent.addEventListener('click', (e) => {
    // get current document ID
    console.log('hmm')
    e.stopPropagation();
    let id = $('.carousel-item').attr('data-id')
    db.collection("checkin").doc(id).delete().then(() => {
        console.log(id + "successfully deleted!");
        $('.carousel-item').attr('data-id')
    }).catch(function (error) {
        console.error("Error removing document: ", error);
    });
});

And this is the erro from the console

Uncaught TypeError: Cannot read property 'addEventListener' of null
    at index.js:123
(anonymous) @ index.js:123
BCDeWitt
  • 4,540
  • 2
  • 21
  • 34
zulfadhli
  • 23
  • 2
  • 6
  • Please include all relevant code. This includes the JS that in your words `returns null`. – Olian04 Sep 17 '19 at 14:09
  • @Olian04 hope the information is enough – zulfadhli Sep 17 '19 at 14:15
  • 1
    I see that you're building the HTML in a `forEach`, but using a static ID attribute. IDs must be unique to the document. Use a class instead. – Heretic Monkey Sep 17 '19 at 14:16
  • @HereticMonkey I have use class as well, same error.. – zulfadhli Sep 17 '19 at 14:34
  • Hi Zulf, it seems that elements are not yet rendered on page before you try to search them with document.querySelector... Sort of async function, try to await to apper in DOM and then put the event listeners. – MilosMarkoni Sep 17 '19 at 14:35
  • Where do you declare variables checkin, card and html? Or are they undeclared global variables? An id of an element must be unique in the document. You can give the buttons a name, and select on that instead. `document.querySelector("[name=delete]")` – some Sep 17 '19 at 14:36
  • Is this using Firebase? – BCDeWitt Sep 17 '19 at 15:15
  • @BDawg yes. this is using firebase – zulfadhli Sep 17 '19 at 15:16

2 Answers2

2

Hopefully I've outlined all the changes I made to get this to work:

  1. As was pointed out in comments, IDs must be unique. Classes are generally better to use as JavaScript (and CSS) hooks. Many people now use a js- prefix for these to help follow the logic from HTML -> JS when maintaining code so I would suggest this.
  2. document.querySelector('.delete') will only get a single element - you need querySelectorAll here, since you will have a delete button for each item.
  3. $('.carousel-item') is (I'm assuming) a jQuery function call. This will get all .carousel-item elements in the document and .attr('data-id') will get the attribute of only the first one. If you wanted to use jQuery here, you would want to go up the DOM from the button element like $(e.target).parent('.carousel-item'). But, since the other code isn't using jQuery, it would be more consistent to use e.target.closest('.js-carousel-item'), imo. Then, to get data-id, we can easily use element.dataset.id.
  4. Don't use globals for this like sao suggested
  5. I'm not sure if data in your example came from a call to db.collection('checkin').get(), but if it was a Promise in your code, instead of the snapshot itself, you would run into problems if your delete button code wasn't nested in the then() callback.
  6. This is more of an optional side-note, but your code could become more readable by refactoring it to use async/await.

I've included a working snippet based on your example below to demonstrate:

;(() => {
  // My attempt at a quick mock of Firebase to make this work as a snippet
  const db = {
    _collections: {
      'checkin': [
        {
          id: 1,
          photo: 'https://images.unsplash.com/photo-1508138221679-760a23a2285b?ixlib=rb-1.2.1&ixid=eyJhcHBfaWQiOjEyMDd9&auto=format&fit=crop&w=1267&q=80',
          title: 'airplane w/ trees',
          description: 'airplane on ground surrounded with trees',
        }, {
          id: 2,
          photo: 'https://images.unsplash.com/photo-1485550409059-9afb054cada4?ixlib=rb-1.2.1&ixid=eyJhcHBfaWQiOjEyMDd9&auto=format&fit=crop&w=701&q=80',
          title: 'head to head',
          description: 'minifigure head lot',
        },
      ],
    },
    collection(key) {
      const c = this._collections[key];
      const doc = (id) => ({
        id,
        data() {
          return c.find(o => o.id === id);
        },
        async delete() {
          const idx = c.findIndex(o => o.id === id);
          c.splice(idx, 1);
        },
      });
      return {
        doc,
        async get() {
          return c.map(o => o.id).map(doc);
        },
      };
    },
  }
  
  const render = () => {
    db.collection('checkin').get().then(snapshot => {
      const cards = snapshot.map(doc => {
        const checkin = doc.data();
        return `
          <div class="js-carousel-item carousel-item fieldId" data-id="${doc.id}">
            <div class="col-12">
              <div class="card checkCard" style="margin: 0 auto;">
                <img src="${checkin.photo}" class="card-img-top" alt="...">
                <button class="js-delete" type="submit" class="btn center">
                <i class="material-icons" style="font-size: 1em;">delete_forever</i>
                </button>
                <div class="card-body">
                  <h5 class="card-title">${checkin.title}</h5>
                  <p class="card-text">${checkin.description}</p>
                </div>
              </div>
            </div>
          </div>`;
      });
      document.querySelector('.js-checkin-list').innerHTML = cards.join('');
  
      // This is the delete button
      const deleteButtons = document.querySelectorAll('.js-delete');
      const deleteHandler = (e) => {
        e.stopPropagation();
        const el = e.target.closest('.js-carousel-item');
        const id = +el.dataset.id;
        db.collection('checkin').doc(id).delete().then(() => {
          console.log(`${id} successfully deleted!`);
        }).catch((error) => {
          console.error('Error removing document: ', error);
        })
        render();
      }
      deleteButtons.forEach(
        btn => btn.addEventListener('click', deleteHandler)
      );
    });
  };
  render();
})();
<div class="js-checkin-list carousel"></div>
BCDeWitt
  • 4,540
  • 2
  • 21
  • 34
1

use onclick inline in your string

for example

<!DOCTYPE html>

<html>

<head>
    <meta charset="UTF-8">
</head>

<body>
  <div id="parentDiv"></div>
</body>

<script>

let element = `<div id="childDiv" onclick="myFunction()">click here</div>`;

document.getElementById("parentDiv").innerHTML = element;


function myFunction() {
    console.log("works every time");
}
</script>
</html>

now the child div gets inserted into the parent and is has an onclick event listener

if you want it to loop, don't loop the addEventListener in JS, loop it in the template literal, in other words, just add this in your string not an extra function.

i just tested it and it works..every...time

have fun!

sao
  • 1,835
  • 6
  • 21
  • 40