Can this function be organized better?

The code is working now, what is the next thing I should do?

https://jsfiddle.net/4my8kxot/

function fadeResetHandler() {
    const body = document.body;
    body.classList.remove("fadingOut");
    resetBackground("body");
    resetCurtains(".with-curtain");
    showAllButtons(".container.hide");
    resetButtons(".outer");

    const onAnimationEnd = fadeResetHandler;
    body.removeEventListener("animationend", onAnimationEnd);
    console.log("remove");
  }

  function resetPage() {
    const body = document.body;
    body.classList.add("fadingOut");

    const onAnimationEnd = fadeResetHandler;
    body.addEventListener("animationend", onAnimationEnd);
  }

The code might be working, but it’s not being done in a correct and proper way.

The fadeResetHandler needs a function parameter called evt, so that document.body in the fadeResetHandler function can be replaced with evt.target

1 Like

Like this? https://jsfiddle.net/ma8pck75/

What do I do after this?

function fadeResetHandler(evt) {
    const body = evt.target;
    body.classList.remove("fadingOut");
    resetBackground("body");
    resetCurtains(".with-curtain");
    showAllButtons(".container.hide");
    resetButtons(".outer");

    const onAnimationEnd = fadeResetHandler;
    body.removeEventListener("animationend", onAnimationEnd);
    console.log("remove");
  }

  function resetPage() {
    const body = document.body;
    body.classList.add("fadingOut");

    const onAnimationEnd = fadeResetHandler;
    body.addEventListener("animationend", onAnimationEnd);
  }

Moving the fadeResetHandler function above the resetPage function is now successful. The more common way to say that is that you have extracted the fadeResetHandler function.

The next thing to do from there is to carry on with the instructions from post #100

As a reminder, the instructions are:

Next is:

the addEventListener can be moved to the init function,

This was my attempt:https://jsfiddle.net/dcg5zfha/

What am I doing wrong, and should be doing instead?

function fadeResetHandler(evt) {
    const body = evt.target;
    body.classList.remove("fadingOut");
    resetBackground("body");
    resetCurtains(".with-curtain");
    showAllButtons(".container.hide");
    resetButtons(".outer");

    const onAnimationEnd = fadeResetHandler;
    body.removeEventListener("animationend", onAnimationEnd);
    console.log("remove");
  }

  function resetPage() {
    const body = document.body;
    body.classList.add("fadingOut");

    /*const onAnimationEnd = fadeResetHandler;    
      body.addEventListener("animationend", onAnimationEnd);*/
  }

  function init(evt) {
    const body = evt.target;
    const exitButtons = document.querySelectorAll(".exit");
    addClickToExit(exitButtons);
    const onAnimationEnd = fadeResetHandler;
    body.addEventListener("animationend", onAnimationEnd);
  }

Sorry no, you’ve done far too much and most of it is wrong.

Can you please just do the simple small thing that has been asked of you, which is to move the addEventListener to the init function. Nothing more.

You will get an console error saying: “Uncaught ReferenceError: body is not defined”
That is when you place a copy of the body variable in the init function.

There are a few other good ways to deal with that.

One way is to add a body variable to the init function. That does though result in us having a body variable in two places, one in resetPage, and one in init. That doesn’t become a problem until we have three such duplicates in the one module. If that occurs we can then move one of them up to a higher level, such as to the top of the module, and use that single body variable from all other places instead.

Another way to solve that is to use a technique called “inline the variable” where the body variable is removed and we instead use what it refers to instead in the code, that being document.body.

However, none of that needs to be solved right now. It’s perfectly good and okay to have two body variables in different places, especially when there’s no good way for one of them to refer to the other. It’s when three-such examples occur though that concern is merited.

After doing that we get a new error message: “Uncaught ReferenceError: onAnimationEnd is not defined”

The onAnimationEnd variable isn’t doing anything useful. We already know that it’s for the animationend event, and it’s just acting as an indirect pass-through to the fadeResetHandler function. Here we get rid of that indirection, and just use fadeResetHandler instead of onAnimationEnd.

The init function is now happy, and there is some cleaning up to do, but the bulk of the work is done.

I did this: https://jsfiddle.net/matgch9d/

You will get an console error saying: “Uncaught ReferenceError: body is not defined”

  function init() {
    const exitButtons = document.querySelectorAll(".exit");
    addClickToExit(exitButtons);
    body.addEventListener("animationend", onAnimationEnd);
  }

That is when you place a copy of the body variable in the init function.

That would mean this: const body = document.body;

Gets added to here? https://jsfiddle.net/pwnf70tr/

function init() {
    const exitButtons = document.querySelectorAll(".exit");
    addClickToExit(exitButtons);
    const body = document.body;
    body.addEventListener("animationend", onAnimationEnd);
  }

After doing that we get a new error message: “Uncaught ReferenceError: onAnimationEnd is not defined”

Here we get rid of that indirection, and just use fadeResetHandler instead of onAnimationEnd.

I did that here: https://jsfiddle.net/u3kgxdrm/

The code is now broken and not functioning properly.

 function init() {
    const exitButtons = document.querySelectorAll(".exit");
    addClickToExit(exitButtons);
    const body = document.body;
    body.addEventListener("animationend", fadeResetHandler );
  }

I am not able to click on all the play svg buttons without it breaking.

The init function is now happy, and there is some cleaning up to do, but the bulk of the work is done.

But the code is not functioning properly, how is that good?

There are I think two main reasons why the code is not functioning. One of them is that your code is still removing the event listener. The update we made to the code is so that only one event listener is needed which means not removing it.

A second reason for trouble is that the fadeResetHandler is triggered on all types of animations that occur, and only one of them is the one that we want to use. The only animation that we want to reset the page on is when the fadingOut animation is being used. There is a problem though, and that is that the fadingOut animation is used when starting a video too. That means that the animationend event is not the correct technique to use. There is instead a much simpler solution that solves all of those problems.

However, first to the cleanup, where the removeeventlistener code is removed, helping to solve one of the issues.

I did that here: https://jsfiddle.net/nqm17exz/

I removed: body.removeEventListener("animationend", onAnimationEnd);

Code:

  function fadeResetHandler(evt) {
    const body = evt.target;
    body.classList.remove("fadingOut");
    resetBackground("body");
    resetCurtains(".with-curtain");
    showAllButtons(".container.hide");
    resetButtons(".outer");

    const onAnimationEnd = fadeResetHandler;
  }
  function resetPage() {
    const body = document.body;
    body.classList.add("fadingOut");

    const onAnimationEnd = fadeResetHandler;
  }
  function init() {
    const exitButtons = document.querySelectorAll(".exit");
    addClickToExit(exitButtons);
    const body = document.body;
    body.addEventListener("animationend", fadeResetHandler);
  }

For guidance: Last working code: https://jsfiddle.net/t19y4d5m/

Original working code, minimal changes made: https://jsfiddle.net/2fyo1s8t/

Which begs the question, if there is a better technique than using animationend,
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/animationend_event

What would be used instead?

Well let’s see what currently happens in the code to cause the code in the handler to run.

  1. resetPage ads a classname of fadingOut to the body
  2. the animationend event triggers
  3. the code in fadeResetHandler is run

Does fadingOut cause any CSS to occur? Yes it does. We can’t remove that.
Can the fadeResetHandler code be run without needing to wait for the event? No it can’t, for it must wait until the end of the animation.

I’ve added a little bit of code at the top of the fadeResetHandler code, to let me see the animationName when the animationend event triggers.

    const name = evt.animationName;
    console.log(name);

There are several animation events that end up triggering the animationend event.

fade
fadebody
curtain1
curtain9
fadingOut
fade

It’s only the fadingOut animation that we want the code to run on, so a guard clause can be handy there. If it’s not fadingOut then we return immediately out of the function.

1 Like

How would I add a guard clause to the code as you suggested?

What is the first thing I should do?

Would I be removing this line from the code?

Is it being changed to something else?

body.removeEventListener("animationend", onAnimationEnd);

What exactly is being added or adjusted in the code?

The animation name is a property on the event object, so you would use evt.animationName to access that.

A guard clause is just an if statement checking if something isn’t what you expect, get the hell outta there.

Yes. That is the whole point of what we’re doing here, so that the unreliable removeEventListener gets removed.

We’ll take this one small step at a time, resolving issues that we come across on the way.

I am heading off on a week-long holiday tomorrow, so depending on your progress there might need to be hiatus over that.

1 Like

This is the first thing I did: https://jsfiddle.net/d7rb6L9o/

  function fadeResetHandler(evt) {
    const name = evt.animationName;
    console.log(name);

Good one. Now do the guard check, by checking if the name is different from “fadingOut” and return out of the function if that is the case.

I have never written a guard check/clause before so I am unsure of how it is written.

or, where it is being placed in the code.

function fadeResetHandler(evt) {
    const name = evt.animationName;
    console.log(name);

    const body = evt.target;
    body.classList.remove("fadingOut");
    resetBackground("body");
    resetCurtains(".with-curtain");
    showAllButtons(".container.hide");
    resetButtons(".outer");

    const onAnimationEnd = fadeResetHandler;

  }

  function resetPage() {
    const body = document.body;
    body.classList.add("fadingOut");

    const onAnimationEnd = fadeResetHandler;
  }

I think that you have written many guard clauses before, you just haven’t recognised them as being such.

Here is the guard clause that you are looking for. Execution of code is not allowed beyond it unless the requirements are met.

if (name !== "fadingOut") {
    return;
}

I would though rename name to be animationName, so that it’s easier to understand what is being done.

Where in the code am I placing this?

  if (animationName !== "fadingOut") {
    return;
}

I did this: https://jsfiddle.net/79p6qv0z/1/

The code is still not working.

function fadeResetHandler(evt) {

    const animationName = evt.animationName;
    console.log(animationName);

    const body = evt.target;
    body.classList.remove("fadingOut");
    resetBackground("body");
    resetCurtains(".with-curtain");
    showAllButtons(".container.hide");
    resetButtons(".outer");

    const onAnimationEnd = fadeResetHandler;

    if (animationName !== "fadingOut") {
      return;
    }
  }

  function resetPage() {
    const body = document.body;
    body.classList.add("fadingOut");

    const onAnimationEnd = fadeResetHandler;
  }

  function init() {
    const exitButtons = document.querySelectorAll(".exit");
    addClickToExit(exitButtons);
    const body = document.body;
    body.addEventListener("animationend", fadeResetHandler);
  }

It’s a guard clause that protects the rest of the code in its area, so it gets placed as early as possible in the code.

There is one last problem and that is the body variable. The evt.target property does not refer to the body element. What does work is using document.body.

However, that is the third time we’ve used document.body in the same area (within the manageUI code), so really reduce that duplication by moving those body variables to a single body variable at the top of the manageUI code instead.

Like this?
Did I do this right?
https://jsfiddle.net/94haL0w1/1/

const manageUI = (function makeManageUI() {
 const body = document.body;

  function fadeResetHandler(evt) {

    const animationName = evt.animationName;
    console.log(animationName);
    if (animationName !== "fadingOut") {
      return;
    }
  
    document.body.classList.remove("fadingOut");
    resetBackground("body");
    resetCurtains(".with-curtain");
    showAllButtons(".container.hide");
    resetButtons(".outer");

    const onAnimationEnd = fadeResetHandler;

  }

  function resetPage() {
    document.body.classList.add("fadingOut");
    const onAnimationEnd = fadeResetHandler;
  }

  function init() {
    const exitButtons = document.querySelectorAll(".exit");
    addClickToExit(exitButtons);
    document.body.addEventListener("animationend", fadeResetHandler);
  }

Next is: const onAnimationEnd = fadeResetHandler;

Can now be deleted from the code? https://jsfiddle.net/94haL0w1/3/

    const onAnimationEnd = fadeResetHandler;

  }

  function resetPage() {
    document.body.classList.add("fadingOut");
    const onAnimationEnd = fadeResetHandler;
  }

Nearly. All places in the manageUI code that use document.body should now just use body instead, as that is referring to the const body statement at the top of the code.

1 Like