Its this good for best practices ? add item and delete

Hello Campers,

I know this been repetitive in the past, but sometimes we want to do or find the right ways of approaching a problem, i just want a feedback regrading best practices, in this type, of scenarios. Ex: i have an input field that adds items and show a delete button, everythings works ok, my only doubt is that if im doing it right by calling/invoking the removeItems() function the right way at the end of the code.

Note: Sometimes developers will want to have an onclick=“removeItems()” on the plain HTML But im not doing it that way i think will be best to have it clean and simple HTML markup, thats why the my js code was written like that.

function removeItems(){
  var removeBtn = document.getElementsByClassName('remove');

  for( var i = 0; i < removeBtn.length; i++ ){
    var elRemoveBtn = removeBtn[i];
    elRemoveBtn.addEventListener('click', function(){
      var item = this.parentNode;
      var parent = item.parentNode;
      return parent.removeChild(item);
    });
  }
};
removeItems(); // invoking the removeItems function. (is this ok ?)

and then calling it on the addItems function

function addItem(){

  var input = document.getElementById("add");
  var myValue = input.value;
  var container = document.getElementById('faves');
  //container.insertAdjacentHTML('afterbegin', `<li>${myValue}</li>`);
  container.innerHTML += '<li><span>'+myValue+'</span><button class="remove" id="removeBtn">Remove</button></li>';
  input.value = "";

  removeItems();  // calling the remove items function.

    };
document.getElementById('btnAdd').addEventListener('click', addItem);

I just want a feedback for best practices and if there is another way of better code it.
codepen link for better undersanding.

Thanks !

In my opinion, you do not need to add an event listener to each button with class=“remove”. One option is to add a single event listener to the ul element with id=“faves” and then detect if the clicked element is a button with class=“removed”.

function addItem() {
  const myValue = input.value;
  const itemElem = document.createElement('li');
  itemElem.innerHTML = `
    <span class="item-text"></span>
    <button class="remove">Remove</button>
  `;
  itemElem.querySelector('.item-text').textContent = myValue;
  container.appendChild(itemElem);
  input.value = '';
}

function removeItem({ target }) {
  if (target && target.nodeName === "BUTTON" && target.classList.contains("remove")) {
    const item = target.parentNode;
    item.parentNode.removeChild(item);
  }
}

const input = document.getElementById("add");
const container = document.getElementById("faves");
document.getElementById("btnAdd").addEventListener("click", addItem);
container.addEventListener("click", removeItem);

This avoids the issue of having to remove an event listener each time an item is removed. You typically want to remove event listeners that are no longer useful (i.e. deleted).

1 Like

It’s not ok to do that :slight_smile:
You keep assigning new eventListeners to existing buttons that already have event listeners - this is bad

Thank you @RandellDawson i will surely do as you mention, that was my only concern maybe i was doing something wrong or good for best practices, thank you so much i will have that in mind as well. This is really helpful, thanks for the feedback. Surely that’s the best way to approach this thanks!.

Thanks! @snigo for your beedback, i will surely have that in mind!.. thannks to many event listeners are not ok if you can have only one. Thanks!.

@imendieta I updated my original code to reflect a more secure version. Whenever you are taking input from a user, you want to avoid using innerHTML if it incorporates the user’s input. The innerHTML would allow a user to add a malicious script to page. I still used innerHTML for the part I control, but then used textContent so the input is purely text.

1 Like

Hi @RandellDawson Oh wow thanks! for letting me know that its good to know that and work with textContent for better script secure page. its good to work with that for me. Also Randell i know i been using ES5 sixtax and not ES6 but i still want to take a bit of practice will good old vanilla js, althought i know i will have to update on const, let, and also template strings to the new back-tick ES6. Thanks for the update Randell. Working hard to be like you :slight_smile:

const and let are good old vanilla js. It is just newer syntax. The sooner you start using ES6, the easier it will be to read through projects using React.

If you have not done so already, I suggest working through the ES6 section of the FCC curriculum. It will give you a decent base understanding and then you can research further outside of FCC.

In this case, the += is not needed. That was just copied over from your code. You would only use the += if you did something like:

  itemElem.innerHTML = '<span class="item-text"></span>';
  itemElem.innerHTML += '<button class="remove">Remove</button>'`;

OK you have this line of code:
itemElem.innerHTML += <span class="item-text"></span><button class="remove">Remove</button>;

when you take away the + and leave the = does the same thing why is that ?

You would use the += if there was already html in the element and you wanted to add more html to the end of the existing html. Since the element’s html was empty, the = works just fine.

I have now updated my code to just use the =.

ok so this can be done with += and = and wont have any impact in any then ?.

No, not in this particular case.

Ok perfect now i get it… thanks @RandellDawson for all your help :slight_smile: