Can this function be organized better?

JavaScript
#95

Did I do everything you asked me to in Post #62?
https://jsfiddle.net/jg2prdch/

const videoPlayer = (function makeVideoPlayer() {

    const tag = document.createElement("script");
    tag.src = "https://www.youtube.com/player_api";
    const firstScriptTag = document.getElementsByTagName("script")[0];
    firstScriptTag.parentNode.insertBefore(tag, firstScriptTag);

    function createResetHandler(player) {
        const resetVideos = document.querySelectorAll(".exit");
        resetVideos.forEach(function resetVideoHandler(video) {
            video.addEventListener("click", function resetVideoHandler() {
                player.destroy();
            });
        });
    }

    function onPlayerReady(event) {
        const player = event.target;
        player.setVolume(100);
        createResetHandler(player);
    }

    function onPlayerStateChange(event) {
        const player = event.target;
        return player;
    }

    function addPlayer(video, playerOptions) {
        playerOptions.videoId = playerOptions.videoId || video.dataset.id;
        playerOptions.events = playerOptions.events || {};
        playerOptions.events.onReady = onPlayerReady;
        playerOptions.events.onStateChange = onPlayerStateChange;

        const player = new YT.Player(video, playerOptions);
        return player;
    }

    return {
        addPlayer
    };
}());

const managePlayer = (function makeManagePlayer() {
    const playerVars = {
        autoplay: 1,
        controls: 1,
        disablekb: 1,
        enablejsapi: 1,
        fs: 0,
        iv_load_policy: 3
    };
    const defaults = {
        height: 360,
        host: "https://www.youtube-nocookie.com",
        playerVars,
        width: 640
    };

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

    function combinePlayerOptions(opts1 = {}, opts2 = {}) {
        const combined = Object.assign({}, opts1, opts2);
        Object.keys(opts1).forEach(function checkObjects(prop) {
            if (typeof opts1[prop] === "object") {
                combined[prop] = Object.assign({}, opts1[prop], opts2[prop]);
            }
        });
        return combined;
    }

    function createPlayer(videoWrapper, playerOptions = {}) {
        const video = videoWrapper.querySelector(".video");
        const options = combinePlayerOptions(defaults, playerOptions);
        return videoPlayer.addPlayer(video, options);
    }

    function createCoverClickHandler(playerOptions) {
        return function coverClickHandler(evt) {
            const cover = evt.currentTarget;
            const wrapper = cover.nextElementSibling;
            show(wrapper);
            const player = createPlayer(wrapper, playerOptions);
            wrapper.player = player;
        };
    }

    function addPlayer(coverSelector, playerOptions) {
        const clickHandler = createCoverClickHandler(playerOptions);
        manageCover.addCoverHandler(coverSelector, clickHandler);
    }

    return {
        add: addPlayer
    };

}());
#96

Yes, it was only to move the defaults out of videoPlayer and into managePlayer instead, in order to achieve a beneficial restructuring. It took a bit of time to understand where you were going wrong, but by stepping back to a previous working stage, we were able to look at what you were next trying to achieve and we managed to resolve things in the end.

That is how troubleshooting tends to work. Step backwards until you get back to something that works, letting you look more closely at what’s going wrong with the next step.

1 Like
#97

3 posts were split to a new topic: Remove manageCover from the managePlayer code

#98

Here I added a fadeOut to the code where the page fades out when the Exit button is clicked.

https://jsfiddle.net/ym3p84s0/

@PaulOB helped me with this, his code had setTimeout in it which I figured out how to remove.

Is there anything in the javascript that can be improved or adjusted?

CSS

.fadingOut .isOpen  {
  animation:fadingOut 5s ;
}
@keyframes fadingOut{
  0%{opacity:1;}
  100%{opacity:0;}
}
}

Javascript

Would you write, or set the code up differently from how it is currently written?

There’s a function in it that needs to be called something.

One thought is calling it, animationEnd, or something different.

  function resetPage() {

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

      document.body.classList.remove("fadingOut");
      resetBackground("body");
      resetCurtains(".with-curtain");
      showAllButtons(".container.hide");
      resetButtons(".outer");
      document.body.removeEventListener("animationend", onAnimationEnd)
    }
    document.body.addEventListener("animationend", onAnimationEnd);
  }

The code uses animationend event:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/animationend_event

Another way it could be written is this way: This uses once: true

  function resetPage() {

    document.body.classList.add("fadingOut");
    document.body.addEventListener("animationend", function needsAName() {

      document.body.classList.remove("fadingOut");
      resetBackground("body");
      resetCurtains(".with-curtain");
      showAllButtons(".container.hide");
      resetButtons(".outer");
    }, {
      once: true
    });
  }

Also, in the code, document.body was previously written as document.querySelector("body")

Then I saw it could be written a different way: document.body
https://developer.mozilla.org/en-US/docs/Web/API/Document/body

Depending on how the code is written, either

document.querySelector("body") or document.body can be used.

document.querySelector("body") would be written this way,

But I don’t know if anything can be improved further from here, if I were to use this setup instead of using document.body

I was thinking, moving the new code into a separate function, but I don’t know if that were to be possible here, and maybe something different would be done instead, I’m not sure.

function resetPage() {

        document.querySelector("body").classList.add("fadingOut");
        const onAnimationEnd = function needsAName() {

            document.querySelector("body").classList.remove("fadingOut");
            resetBackground("body");
            resetCurtains(".with-curtain");
            showAllButtons(".container.hide");
            resetButtons(".outer");
            document.querySelector("body").removeEventListener("animationend", onAnimationEnd)
        }
        document.querySelector("body").addEventListener("animationend", onAnimationEnd);
    }

I was thinking something similar to how these were done:

    function resetBackground(backgroundSelector) {
        const allBackgrounds = document.querySelectorAll(backgroundSelector);

        function showBackground(background) {
            background.classList.remove("bg1");
        }
        allBackgrounds.forEach(showBackground);
    }

    function resetCurtains(curtainSelector) {
        const allCurtains = document.querySelectorAll(curtainSelector);

        function showCurtain(curtain) {
            curtain.classList.remove("active");
        }
        allCurtains.forEach(showCurtain);
    }

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

Moving these out to a function, and calling it something.

document.querySelector("body").classList.remove("fadingOut");
document.querySelector("body").removeEventListener("animationend", onAnimationEnd)
document.querySelector("body").addEventListener("animationend", onAnimationEnd);

Then doing something like this, I’m not sure how it would work in the code though.
onAnimationEnd("body");

#99

With the code written this way, can document be moved out to a separate function, where a function variable would be placed inside resetPage() instead?

https://jsfiddle.net/6pvebm08/

    function resetPage() {

        document.querySelector("body").classList.add("fadingOut");
        const onAnimationEnd = function needsAName() {

            document.querySelector("body").classList.remove("fadingOut");
            resetBackground("body");
            resetCurtains(".with-curtain");
            showAllButtons(".container.hide");
            resetButtons(".outer");
            document.querySelector("body").removeEventListener("animationend", onAnimationEnd)
        }
        document.querySelector("body").addEventListener("animationend", onAnimationEnd);
    }
#100

It’s normally not suitable to replace document with other things. That is an indirection (referencing using a reference instead of the thing itself), and leads to too much confusion.

Using a variable at the start of the resetPage function is suitable though.

const body = document.querySelector("body");

That way you can just use body instead of document.querySelector(“body”)

In regard to the function name, it helps to ask what does the function do? It’s an event handler that triggers after the fade animation has ended. The animation end part is already taken care off by the event name of “animationend”, so the rest of what it does is explained by being a fade reset handler. Calling the function fadeResetHandler looks to be a most suitable name.

The removeEventListener that you’ve used there is poorly supported by some web browsers. Instead of adding and removing the event listener, it’s best to add the event listener only once, usually from an init function, and don’t fiddle with it afterwards. To achieve that the fadeResetHandler function cannot be inside of the resetPage function. That fadeResetHandler function needs to be moved out of that function, possibly just above the resetPage function. That way the addEventListener can be moved to the init function, and the fadeResetHandler function can still be accessed from there. That way the removeEventHandler statement can be removed, helping to make your code more reliable across different web browsers.

1 Like
#101

First:

This:
document.querySelector("body").classList.add("fadingOut");

Becomes this:
const body = document.querySelector(“body”).classList.add(“fadingOut”);

Second: This becomes:
const onAnimationEnd = function needsAName() {

This:
const onAnimationEnd = function fadeResetHandler () {

Code Still Works: https://jsfiddle.net/hm5zfcrw/

function resetPage() {

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

        document.querySelector("body").classList.remove("fadingOut");
        resetBackground("body");
        resetCurtains(".with-curtain");
        showAllButtons(".container.hide");
        resetButtons(".outer");
        document.querySelector("body").removeEventListener("animationend", onAnimationEnd)
    }
    document.querySelector("body").addEventListener("animationend", onAnimationEnd);
}

From there, what do I do next to where the code will continue to stay working?

or, am I supposed to be using errors to get it to work?

I tried doing this next, but the code stopped working here: https://jsfiddle.net/91txmvzf/

 const body = document.querySelector("body").classList.add("fadingOut");

  const onAnimationEnd = function fadeResetHandler() {
    function resetPage() {

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

Then I did this:

I moved addEventListener to the init function.

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

I tried to follow the instructions you gave me to implement, but I couldn’t get it to work in the code.

Are there other things I am supposed to do also?

#102

When unexpected problems occur, that’s your signal that you’re biting off more than you can chew. That’s when you should step back to what works, and take a closer look at what was going wrong.

#103

From this point in the code, what should I do next? https://jsfiddle.net/hm5zfcrw/

You told me to place:
const onAnimationEnd = function fadeResetHandler () {

Above: function resetPage() {

But, that’s where the code stops working.

Are there certain things I should be doing in steps?

This seems complicated, would you be able to put together steps I can follow to get this to work?

function resetPage() {

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

        document.querySelector("body").classList.remove("fadingOut");
        resetBackground("body");
        resetCurtains(".with-curtain");
        showAllButtons(".container.hide");
        resetButtons(".outer");
        document.querySelector("body").removeEventListener("animationend", onAnimationEnd)
    }
    document.querySelector("body").addEventListener("animationend", onAnimationEnd);
}
#104

There’s no point going further yet because you haven’t yet achieved the following:

2 Likes
#105

This is what you wanted me to do: https://jsfiddle.net/6cudwg2p/

What should the next thing I do be?

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

      document.querySelector("body").classList.remove("fadingOut");
      resetBackground("body");
      resetCurtains(".with-curtain");
      showAllButtons(".container.hide");
      resetButtons(".outer");
      document.querySelector("body").removeEventListener("animationend", onAnimationEnd);
    }
    document.querySelector("body").addEventListener("animationend", onAnimationEnd);
  }
#106

I also said:

which uses that body variable, and hasn’t been done by you yet.

#107

Is this what you wanted me to do? https://jsfiddle.net/89mbv7wg/

Can progress be made from here?

function resetPage() {

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

    const onAnimationEnd = function fadeResetHandler() {

      document.body.classList.remove("fadingOut");
      resetBackground("body");
      resetCurtains(".with-curtain");
      showAllButtons(".container.hide");
      resetButtons(".outer");
      document.body.removeEventListener("animationend", onAnimationEnd);
    }
    document.body.addEventListener("animationend", onAnimationEnd);
  }
#108

No, that isn’t what I want you to do.

What I want you to do is to use the body variable throughout the rest of the code in that functon…

#110

I think this is what you want me to do:

Also, can I use this: const body = document.body;

Instead of this? const body = document.querySelector("body");

Is there a difference?

They both mean the same thing, right, just one is shorter?

or, should I really be using this instead? document.querySelector("body");

For what I am going to be doing, does it matter which way it’s written, or either way is fine?

Then, what am I supposed to do next in the code?

https://jsfiddle.net/mynbt7Ls/

function resetPage() {

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

    const onAnimationEnd = function fadeResetHandler() {

      body.classList.remove("fadingOut");
      resetBackground("body");
      resetCurtains(".with-curtain");
      showAllButtons(".container.hide");
      resetButtons(".outer");
      body.removeEventListener("animationend", onAnimationEnd);
      console.log("remove fadeResetHandler");
    }

    body.addEventListener("animationend", onAnimationEnd);
  }
#111

Yes, document.body can be used instead.

That has already been stated in post #100

1 Like
#112

I understand that but, this is at the spot where I am having a tremendous amount of difficulty.

Last working code: https://jsfiddle.net/mynbt7Ls/

Following the instructions, I would be doing this:

Where the code does not work: https://jsfiddle.net/73b41oeu/

The code falls apart when const onAnimationEnd = function fadeResetHandler() {

is placed above: function resetPage() {

This: body.classList.add("fadingOut");

Has to be placed above: const onAnimationEnd = function fadeResetHandler() {

Then: const body = document.body; would need to be placed above both of those.

Which I did in the attempted code.

Attempted Code:

  const body = document.body;

  body.classList.add("fadingOut");

  const onAnimationEnd = function fadeResetHandler() {

    function resetPage() {

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

  }

Then: body.addEventListener("animationend", onAnimationEnd);
Gets added to the init function.

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

Is there way to do this in steps where the code continues to stay working?

or can this be done in steps where errors are used to get to the working code?

#113

Okay, so step back before doing that and take it slower.

Instead of directly moving the function out, break it down into smaller steps. First make fadeResetHandler a separate function declaration inside of the resetPage function, and have the const onAnimationEnd line refer to that local function declaration of fadeResetHandler instead.

When that is working, you can then move the fadeResetHandler function out of the resetPage function, and check if things are still working.

#114

I did this: First make fadeResetHandler a separate function declaration inside of the resetPage function

But what does this mean? and have the const onAnimationEnd line refer to that local function declaration of fadeResetHandler instead.

How do I have const refer to the local function declaration? I don’t understand how to do that.

I don’t think making const onAnimationEnd = {}; a global variable is what you wanted me to do.

Doing this is not right either: const onAnimationEnd = fadeResetHandler;

This is wrong also: const onAnimationEnd = {fadeResetHandler};

None of those ways I tried work in the code.

https://jsfiddle.net/1vt3z8eu/

  function resetPage() {

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

    function fadeResetHandler() {
      const onAnimationEnd = {};
      
      body.classList.remove("fadingOut");
      resetBackground("body");
      resetCurtains(".with-curtain");
      showAllButtons(".container.hide");
      resetButtons(".outer");
      body.removeEventListener("animationend", onAnimationEnd);
      console.log("remove fadeResetHandler");
    }
    body.addEventListener("animationend", onAnimationEnd);
  }
#115

That const statement doesn’t belong in the fadeResetHandler function. It should be below the function where it assigns fadeResetHandler to onAnimationEnd.