Can this function be organized better?

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

Is this everything? https://jsfiddle.net/afrqgzos/

I put the javascript through jslint and everything is fine.

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

        const animationName = evt.animationName;
        console.log(animationName);
        if (animationName !== "fadingOut") {
            return;
        }

        body.classList.remove("fadingOut");
        resetBackground("body");
        resetCurtains(".with-curtain");
        showAllButtons(".container.hide");
        resetButtons(".outer");
    }

    function resetPage() {
        body.classList.add("fadingOut");
    }

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

Yep. The one thing that was done in that whole process was to replace a combination of addEventListener +removeEventListener with only a single addEventListener.

Everything else was responding to issues from that restructure, using best-practice techniques.

It’s easier to make those adjustments when code is well-structured. Poorly structured code becomes a nightmare to make those adjustments. Fortunately here the code is mostly well structured, so it wasn’t too tough to make those adjustments.

A fundamental principle when restructuring code is to test early, test often. When small steps are taken each time before testing, it tends to only be a small and easy update to get things working again.

1 Like

There’s only one further thing that I would do in terms of the restructure with this event handler, and that is to split up the fadeResetHandler. Right now it’s checking if something should be done, then doing that thing. The handler should only do the checking before calling a separate function. That means moving the rest of the code after the if statement into a separate function.

The fadeResetHandler is actually an animationEndHandler and should be renamed to that instead.
That allows us to move the rest of the code after the if statement into a separate function called fadeReset.

That way instead of having fadeReset being called after the if statement:

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

We can invert the condition in the if statement and have fadeReset being called from inside of it.

if (animationName === "fadingOut") {
    fadeReset();
}

Why that is beneficial is that it makes it easier to extend the code later on, should other animations other than fadingOut require other code being added there too.

1 Like

Is this what you wanted me to do?

Did I do this right?

https://jsfiddle.net/hbfszoqp/

  function animationEndHandler(evt) {

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

    if (animationName === "fadingOut") {
      fadeReset();
    }
  }

  function fadeReset() {
    body.classList.remove("fadingOut");
    resetBackground("body");
    resetCurtains(".with-curtain");
    showAllButtons(".container.hide");
    resetButtons(".outer");
  }
1 Like

Yes that’s right. It is the proper job of an event handler to be like a policeman at an intersection, to direct traffic where it needs to go. That way complexity is dramatically reduced.

2 Likes

I’m wondering if Paul Wilkins will be included in the copyright for this.
It is a great tutorial but I always found that I was more likely to retain information when I solved a problem myself rather than having the solution served up on a platter.

1 Like

Somehow I doubt it, but it would be an appropriate attribution if it occurred.

That’s why I now try to explain things in a descriptive detail, so that appropriate terminology and concepts can be engaged with. Because I am not on the clock, I have as much time as is necessary to try and help those concepts to be learned.

2 Likes

You should provide your own custom badge for inclusion on the associated website as a token of gratitude for the time you have taken to help people like this.

To be able to have the buttons fade-out when they are clicked on, animationend was added to the manageCover Function.

I had help with this, just to get the functionality working.

This is how the coverClickHandler function originally looked without any changes to it.

  function coverClickHandler(evt) {
    hideAll(config.containers);
    resetPage();
    markAsPlayed(evt.currentTarget);
    const cover = evt.currentTarget;
    showCovers(cover);
  }

This was one attempt at trying to get the code to work.

function animationEndHandler(evt) {
    const animationName = evt.animationName;
    if (animationName === "initial-fade") {
      swapBackgrounds(evt);
    }
  }

  function swapBackgrounds(evt) {
    body.classList.remove("initial-fade");
    hideAll(config.containers);
    resetPage();
    markAsPlayed(evt.currentTarget);
    const cover = evt.currentTarget;
    showCovers(cover);
  }

  function resetPage() {
    body.classList.add("initial-fade");
  }

  function coverClickHandler() {
    resetPage();
  }

Here is what got it to work.

Working Code: https://jsfiddle.net/9w1bfqvd/

When you click on the buttons, you can see they now fade out.

How can the javascript be fixed so it is proper code?

This is the code that was added to it to get it to work.

This was the added CSS

body.initial-fade {
  animation: initial-fade 800ms ease forwards;
}

@keyframes initial-fade {
  to {
    opacity: 0;
  }
}

This was the added: Javascript

const manageCover = (function makeManageCover() {
  var originalEvent = "";


  function animationEndHandler2(evt) {
    const animationName = evt.animationName;
    if (animationName === "initial-fade") {
      coverClickHandlerContinue(originalEvent);
    }
  }

  function coverClickHandlerContinue(originalEvent) {
    console.log(originalEvent);
    body.classList.remove("initial-fade");
    hideAll(config.containers);
    resetPage();
    markAsPlayed(originalEvent);
    const cover = originalEvent;
    showCovers(cover);
  }

  function coverClickHandler(evt) {
    originalEvent = evt.currentTarget;
    body.classList.add("initial-fade");
  }

  function init(selectors) {
    body.addEventListener("animationend", animationEndHandler2);
  }

That originalEvent variable really is a problem, so let’s see what we can do about that.

In the coverClickHandlerContinue function the originalEvent variable is renamed to be cover. It’s not a cover though because the showCovers function calls it a playButton, and from that playButton the showCovers function gets the cover.

So the first change that needs to be done is that the coverClickHandlerContinue function parameter should be renamed to be playButton, with code in that function being renamed to use playButton instead.

The showCovers function doesn’t show all of the covers either. Instead it just shows one cover, so the showCovers function needs to be renamed to showCover instead. The originalEvent variable can also be more correctly renamed to be currentPlayButton instead.

The removal of initial-fade shouldn’t be in the coverClickHandlerContinue function either. Instead it’s more appropriate for that to be moved into the if statement that checks for initial-fade.

The coverClickHandlerContinue is now more easily understood to do things that are needed when showing the cover. We can move all of the coverClickHandlerContinue code (all except for the showCover line of course) into the showCover function. Any references to coverClickHandlerContinue can now be renamed to showCover, allowing us to delete the coverClickHandlerContinue function.

1 Like

Following your instructions, this is what I have:
https://jsfiddle.net/d04ov5g9/

const manageCover = (function makeManageCover() {
  var currentPlayButton = "";

  function showCover(playButton) {
    hideAll(config.containers);
    resetPage();
    markAsPlayed(playButton);
    const cover = playButton.parentElement;
    cover.classList.add("active");
    show(cover);
  }

  function animationEndHandler2(evt) {
    const animationName = evt.animationName;

    if (animationName === "initial-fade") {
      body.classList.remove("initial-fade");
      showCover(currentPlayButton);
    }
  }

  function coverClickHandler(evt) {
    currentPlayButton = evt.currentTarget;
    body.classList.add("initial-fade");
  }

  function init(selectors) {
    body.addEventListener("animationend", animationEndHandler2);
  }

Good one. That animationEndHandler2 shouldn’t have a number on it either. There doesn’t seem to be an animationEndHandler that already exists, so rename animationEndHandler2 to be animationEndHandler

1 Like

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

Will this global variable be removed, or is that staying?
let currentPlayButton = {};

One wasn’t needed with the manageUI function, not sure why it would be needed in one and not the other. Maybe they both can’t be set up the same way.

 function animationEndHandler(evt) {
    const animationName = evt.animationName;

    if (animationName === "initial-fade") {
      body.classList.remove("initial-fade");
      showCover(currentPlayButton);
    }
  }

  function init(selectors) {
    body.addEventListener("animationend", animationEndHandler);
  }

How animationend was added to each.

animationEndHandler added to manageCover

Maybe this one needs let currentPlayButton = {};

const manageCover = (function makeManageCover() {
      const body = document.body;
      let currentPlayButton = {};

      const animationName = evt.animationName;
      if (animationName === "initial-fade") {
        body.classList.remove("initial-fade");
        showCover(currentPlayButton);
      }
    }

    function coverClickHandler(evt) {
      currentPlayButton = evt.currentTarget;
      body.classList.add("initial-fade");
    }

    function init(selectors) {
      body.addEventListener("animationend", animationEndHandler);
    }

animationEndHandler added to manageUI

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

          function animationEndHandler(evt) {

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

            if (animationName === "fadingOut") {
              fadeReset();
            }
          }

          function fadeReset() {
            body.classList.remove("fadingOut");
            resetBackground("body");
            resetCurtains(".with-curtain");
            showAllButtons(".hide");
            resetButtons(".outer");
          }

          function resetPage() {
            body.classList.add("fadingOut");
          }

          function exitClickHandler() {
            resetPage();
          }

          function init() {
            body.addEventListener("animationend", animationEndHandler);
          }

The currentPlayButton variable is a temporary storage place, so that while waiting for the animation to end, it can be remembered which play button was activated. That’s the only reason for the use of currentPlayButton

1 Like

Another possible solution is to add a dataset value onto the body, allowing us to remove the currentPlayButton variable. That alternative solution could be explored too if you’re up for it.

1 Like

What would the alternative solution be?

You mean this:

Another possible solution is to add a dataset value onto the body, allowing us to remove the currentPlayButton variable.

How would that be done?

What would the steps be to do that?

I was reading this:

I thought that it would be easy, but on further investigation dataset only lets you store a string value. It’s not possible to store a reference to a page element in a dataset, storing classname information is a terrible idea because parts of that change frequently.

Using a unique identifier is a possible solution but that means adding even more to the HTML code and other scripting code to get the appropriate unique id. Using a dataset is a bad motivation to start using unique ids when they’re not being already used, because we already have a good working solution, so datasets don’t look to be a viable solution.

That’s okay - for figuring out what doesn’t work is an important part of the process as well.

1 Like

10 posts were split to a new topic: Changes to cope with separating HTML players and covers

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.