Improving manageCover code

JavaScript
1

This is the part that is being worked on here.

Organizing, improving it, making it better.
https://jsfiddle.net/6Lhc1saw/

(function manageCovera(d) {
  const allPlayButtons = d.querySelectorAll(".thePlay");
  const allContainers = d.querySelectorAll(".container");

  function show(el) {
    el.classList.remove("hide");
  }

  function hide(el) {
    el.classList.add("hide");
  }

  function addClickToButtons() {
    for (let i = 0; i < allPlayButtons.length; i++) {
      allPlayButtons[i].addEventListener("click", coverClickHandler);
    }
  }

  function coverClickHandler(evt) {
    const cover = evt.currentTarget.parentElement;
    for (let i = 0; i < allContainers.length; i++) {
      allContainers[i].classList.add("hide");
    }
    cover.classList.add("active");
    cover.classList.remove("hide");
  }

  addClickToButtons();
})(document);
2

The function shouldn’t be called manageCovera because there should not exist manageCoverb and manageCoverc and manageCoverd and manageCovere.

Instead of having multiple different functions, it’s much better to have just the one manageCover function that can do what all of them need.

1 Like
3

Only 1 manageCover
https://jsfiddle.net/nd8ftcwu/

(function manageCover(d) {
  const allPlayButtons = d.querySelectorAll(".thePlay");
  const allContainers = d.querySelectorAll(".container");

  function show(el) {
    el.classList.remove("hide");
  }

  function hide(el) {
    el.classList.add("hide");
  }

  function addClickToButtons() {
    for (let i = 0; i < allPlayButtons.length; i++) {
      allPlayButtons[i].addEventListener("click", coverClickHandler);
    }
  }

  function coverClickHandler(evt) {
    const cover = evt.currentTarget.parentElement;
    for (let i = 0; i < allContainers.length; i++) {
      allContainers[i].classList.add("hide");
    }
    cover.classList.add("active");
    cover.classList.remove("hide");
  }

  addClickToButtons();
})(document);
4

You will find that JSLint refuses to allow for loops, because there are much better solutions out there.
In this case it is to use the forEach method on the arrays instead.

5

Here’s the addClickToButtons function

  function addClickToButtons() {
    for (let i = 0; i < allPlayButtons.length; i++) {
      allPlayButtons[i].addEventListener("click", coverClickHandler);
    }
  }

After using forEach it becomes this instead:

  function addClickToButtons() {
    allPlayButtons.forEach(function addEventHandler(playButton) {
      playButton.addEventListener("click", coverClickHandler);
    });
  }

Here’s the coverClickHandler function

  function coverClickHandler(evt) {
    const cover = evt.currentTarget.parentElement;
    for (let i = 0; i < allContainers.length; i++) {
      allContainers[i].classList.add("hide");
    }
    cover.classList.add("active");
    cover.classList.remove("hide");
  }

And here it improved by using forEach

  function coverClickHandler(evt) {
    allContainers.forEach(function hideContainer(container) {
      container.classList.add("hide");
    });
    const cover = evt.currentTarget.parentElement;
    cover.classList.add("active");
    cover.classList.remove("hide");
  }
6

I did that:
https://jsfiddle.net/pczng2va/

(function manageCover(d) {
  const allPlayButtons = d.querySelectorAll(".thePlay");
  const allContainers = d.querySelectorAll(".container");

  function show(el) {
    el.classList.remove("hide");
  }

  function hide(el) {
    el.classList.add("hide");
  }

  function addClickToButtons() {
    allPlayButtons.forEach(function addEventHandler(playButton) {
      playButton.addEventListener("click", coverClickHandler);
    });
  }

  function coverClickHandler(evt) {
    allContainers.forEach(function hideContainer(container) {
      container.classList.add("hide");
    });
    const cover = evt.currentTarget.parentElement;
    cover.classList.add("active");
    cover.classList.remove("hide");
  }

  addClickToButtons();
})(document);
7

The allPlayButtons and allContainers variables at the top have no need to be there at all. They are only used by one set of code, so keep them together with that code.

8

Like this?
https://jsfiddle.net/pczng2va/1/

 function addClickToButtons() {
    const allPlayButtons = d.querySelectorAll(".thePlay");
    allPlayButtons.forEach(function addEventHandler(playButton) {
      playButton.addEventListener("click", coverClickHandler);
    });
  }

  function coverClickHandler(evt) {
    const allContainers = d.querySelectorAll(".container");
    allContainers.forEach(function hideContainer(container) {
      container.classList.add("hide");
    });
    const cover = evt.currentTarget.parentElement;
    cover.classList.add("active");
    cover.classList.remove("hide");
  }
1 Like
9

Can the d be changed to something more suitable?

(function manageCover(d)

const allPlayButtons = d.
const allContainers = d.

Will that be removed?

10

The coverClickHandler clearly does two different things, one is to hide the containers and the other is to show the cover. You should move those two different things out to separate functions, so that the coverClickHandler becomes simplified to:

function coverClickHandler(evt) {
  hideContainers();
  showCovers(evt.currentTarget);
}

with support functions of:

function hideContainers() {
    const allContainers = d.querySelectorAll(".container");
    allContainers.forEach(function hideContainer(container) {
      container.classList.add("hide");
    });
}
function showCovers(playButton) {
    const cover = playButton.parentElement;
    cover.classList.add("active");
    cover.classList.remove("hide");
}
11

I got it:
https://jsfiddle.net/rc38k1uv/

12

There are other improvements that can be made, but those are about making future work with the code easier to achieve.

13

Those should be highlighted, not in gray.

Also,

})(document);

I don’t think should be in the code.

14

The grey is because it can’t find the functions yet. Those functions are further down. Move the coverClickHandler function below them. Click handler functions should be at the end of the code anyway.

1 Like
15

You can remove that document part, but then you’ll also need to replace d inside of the code with document.

1 Like
16

If I remove document, the code stops working.
https://jsfiddle.net/rc38k1uv/4/

 function addClickToButtons() {
    const allPlayButtons = document.querySelectorAll(".thePlay");
    allPlayButtons.forEach(function addEventHandler(playButton) {
      playButton.addEventListener("click", coverClickHandler);
    });
  }

  function hideContainers() {
    const allContainers = document.querySelectorAll(".container");
    allContainers.forEach(function hideContainer(container) {
      container.classList.add("hide");
    });
  }

  function showCovers(playButton) {
    const cover = playButton.parentElement;
    cover.classList.add("active");
    cover.classList.remove("hide");
  }
  
  function coverClickHandler(evt) {
    hideContainers();
    showCovers(evt.currentTarget);
  }

  addClickToButtons();
  
})(document);
17

In what way does it stop working?

18

I think it needs this at the bottom of it, right?

or no?

  const cover = document.querySelector("");
  cover.addEventListener("click", coverClickHandler);
}());
19

Like this?
https://jsfiddle.net/zqpse9kd/2/

  function coverClickHandler(evt) {
    hideContainers();
    showCovers(evt.currentTarget);
  }

  addClickToButtons();
 
}());
20

Can you please show where you removed document from, for it to stop working?