Can this function be organized better?

JavaScript
81

This can be written so many different ways I’m confused and don’t know which should be used.

If I do this:
This would then be deleted: const players = [];

Do you want me to do this? This works.
https://jsfiddle.net/vsqezghc/

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

The code originally used this: Code 1 https://jsfiddle.net/k24ecz8t/

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

You wanted me to do this: Code 2 https://jsfiddle.net/Lukx134r/

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

Where I get this in jslint:

Unused ‘player’.
const player = new YT.Player(video, playerOptions);

Full Code 1
const players = [];

    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);
        players.push(player);
    }

Full Code 2
const players = [];

  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);
    }

Should one of these be used?

Code works using this: https://jsfiddle.net/sczvwo2f/

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

Code works also using this: https://jsfiddle.net/z0f6sd87/

    players.push = new YT.Player(video, playerOptions);
  }
82

I’m thinking I should use this one because then I would be able to delete the global variable.

const players = [];

https://jsfiddle.net/vsqezghc/

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

What I want you to do is to get to the last stage that works, and tell me about the next stage that you have trouble with. Can you supply that information?

86

You wanted me to do this:

jslint error code / Code works, just that jslint error.
https://jsfiddle.net/Lukx134r/

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

Where I get this in jslint:

Unused ‘player’.
const player = new YT.Player(video, playerOptions);

I changed the code to this:

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

Now the code works: https://jsfiddle.net/vsqezghc/

    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;
    }

And the global variable gets deleted: const players = [];

I am up to removing manageCover from the manangePlayer function.

If that is what is next to do?

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

or this can be deleted: const player =

And the code works like this:
https://jsfiddle.net/1bx7goak/1/

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

        new YT.Player(video, playerOptions);
    }
88

There is no issue with the code working.

The only issue there is is:

What should this last line be where jslint doesn’t have a problem with it.

jslint likes how these are written.

Code 1
https://jsfiddle.net/syac7uno/

        new YT.Player(video, playerOptions);
    }

Code 2
https://jsfiddle.net/vsqezghc/

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

Written those ways, the global variable can be deleted:
const players = [];

89

It looks like you have deleted the rest of that addPlayer function below the player line. Please restore the code that you deleted below it.

90

Both codes work where jslint doesn’t have a problem.

Code 1
https://jsfiddle.net/syac7uno/

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

        new YT.Player(video, playerOptions);
    }

Code 2
https://jsfiddle.net/vsqezghc/

    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;
    }
91

The managePlayer code expects to get a player from that addPlayer function.

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

That player is then added to the wrapper, so that we can later on access the player.

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

Just because it doesn’t “break anything” right now, doesn’t mean that it shouldn’t be there.

The addPlayer function needs to return the player that it made.

2 Likes
92

With that information, if I understand you correctly.

Of those 2 codes, I should be using this one?

The one with the return player?

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

https://jsfiddle.net/vsqezghc/

    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;
    }
1 Like
93

Is the plan still to remove manageCover from the managePlayer function?

If yes, how would that be done?

https://jsfiddle.net/vsqezghc/

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

That will be happening after everything from my previous instructions has been completed.

1 Like
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.