JavaScript JavaScript and the DOM Traversing the DOM Getting the First and Last Child

Grzegorz Zielinski
Grzegorz Zielinski
5,838 Points

Improved solution for buttons challenge - what do you think?

Ok, I know it's been a while but I've made some more improvements to my JS code yesterday following your advice, and using a new skills I've learned meanwhile (callbacks) :)

Here is my new improved solution, I wonder what you think about it:

const descriptionInput = document.querySelector("input.description");
const descriptionP = document.querySelector("p.description");
const descriptionButton = document.querySelector("button.description");

const toggleList = document.querySelector("#toggleList");
const list = document.querySelector(".list");
const ulList = list.querySelector("ul");

const addItemInput = document.querySelector("input.addItemInput");
const addItemButton = document.querySelector("button.addItemButton");

const firstListElement = document.querySelector(".up");
const lis = ulList.children;
const firstPosition = ulList.firstElementChild;
const lastPosition = ulList.lastElementChild;


// My part of code starts here
function attachListItemButtons(li) {  // checks if element is first/last element of ulList and then - if not, attaches all the buttons, if some of the condition is true - skips that part of a function. Also checks if "li" doesn't have "null" valule which is useful if we remove all the elements and it's only one left on page.
  if (li !== null) {
    if (li !== ulList.firstElementChild) {
      let up = document.createElement("button");
      up.className = "up";
      up.textContent = "Up";
      li.appendChild(up);
    }
    if (li !== ulList.lastElementChild) {
      let down = document.createElement("button");
      down.className = "down";
      down.textContent = "Down";
      li.appendChild(down);
    }
    let remove = document.createElement("button");
    remove.className = "remove";
    remove.textContent = "Remove";
    li.appendChild(remove);
  }
}

function firstLastBGColor() { //Same as before - gives different colors to last/first element
  let first = ulList.firstElementChild;
  let last = ulList.lastElementChild;
  for (let i = 0; i < lis.length; i += 1) {
    lis[i].style.background = "white";
    first.style.background = "lightblue";
    last.style.background = "lightpink";
    ulList.firstElementChild.firstElementChild.style.background = "#508abc";
  }
}

function removeButtons(element) { // removes all the buttons from "element" and also checks if it doesn't have "null" value (same reason as above with "attachListItemButtons"
  if (element !== null) {
    var child = element.lastElementChild;
    while (child) {
      element.removeChild(child);
      child = element.lastElementChild;
    }
  }
}

function removeAndAttach(func1, func2) { // callback which uses two functions and simple loop for 2nd, 3rd, 4th, and 5th arument
  for (i = 2; i < 6; i++) {
    func1(arguments[i]);
    func2(arguments[i]);
  }
}

for (let i = 0; i < lis.length; i += 1) { // generates all the buttons when the page loads for first time
  attachListItemButtons(lis[i]);
}

firstLastBGColor(); // generate colors when the page loads for first time

list.addEventListener("click", (event) => { // event which triggers functions below
  let first = ulList.firstElementChild; // variables aiming first/second/penultimate/last element on page
  let second = ulList.firstElementChild.nextElementSibling;
  let penultimate = ulList.lastElementChild.previousElementSibling;
  let last = ulList.lastElementChild; 
  removeAndAttach( // callback function used here to first remove and then to attach new buttons to each of argument.
    removeButtons,
    attachListItemButtons,
    first,
    second,
    penultimate,
    last
  );
  firstLastBGColor();
});

// End of my part of code

ulList.addEventListener("click", (event) => {
  if (event.target.tagName == "BUTTON") {
    if (event.target.className == "remove") {
      let li = event.target.parentNode;
      let ul = li.parentNode;
      ul.removeChild(li);
    }
    if (event.target.className == "up") {
      let li = event.target.parentNode;
      let prevLi = li.previousElementSibling;
      let ul = li.parentNode;
      if (prevLi) {
        ul.insertBefore(li, prevLi);
      }
    }
    if (event.target.className == "down") {
      let li = event.target.parentNode;
      let nextLi = li.nextElementSibling;
      let ul = li.parentNode;
      if (nextLi) {
        ul.insertBefore(nextLi, li);
      }
    }
  }
});

descriptionButton.addEventListener("click", () => {
  descriptionP.innerHTML = `<h2>${descriptionInput.value}:</h2>`;
  descriptionInput.value = "";
});

toggleList.addEventListener("click", () => {
  if (list.style.display == "none") {
    list.style.display = "block";
    toggleList.textContent = "Hide List";
  } else {
    list.style.display = "none";
    toggleList.textContent = "Show List";
  }
});

addItemButton.addEventListener("click", () => {
  let ul = document.querySelector("ul");
  let li = document.createElement("li");
  li.textContent = addItemInput.value;
  attachListItemButtons(li);
  ul.appendChild(li);
  addItemInput.value = "";
});

I know there is still one more thing I could improve - when there is less than 4 elements in ulList removeAndAttach callback function removes and attach buttons to same elements twice (when for the example there is 3 elements then 2nd one is taken as a "second " and "penultimate" variable so in this case buttons are removed->generated->removed->generated again) , but I need more time to sort it out, so that's going to be my next task :)

Have you got any other advice for me, or ideas for improvements?

Thanks!