Adding Const defaultPlayerVars =

[Paul_Wilkins]

That comment about settings having changed means that there’s an opportunity to improve the code. What if there was a function that we could give settings to, and it split out the options and playerVars for us? Then we wouldn’t need to be concerned with such issues.

    function separateOptions(settings) {
        const options = {
            playerVars: {}
        };
        const optionParams = ["width", "height", "videoid", "host"];
        Object.entries(settings).forEach(function separate([param, value]) {
            if (optionParams.includes(param)) {
                options[param] = value;
            } else {
                options.playerVars[param] = value;
            }
        });
        return options;
    }

We can then get the options and playerVars separated, and combine them with the defaults:

    function createPlayerOptions(settings) {
        const options = separateOptions(settings);
        const playerOptions = Object.assign({}, defaults, options);
        playerOptions.playerVars = Object.assign(
            {},
            defaults.playerVars,
            options.playerVars
        );
        return playerOptions;
    }

That updated code is a lot easier to understand now. https://jsfiddle.net/quabrytc/1/

[Paul_Wilkins]

To help simplify things I’ve moved the code out to local files, with a different file for each section of code. I now have the following script files:

• spinner.js
• manage-cover.js
• video-player.js
• add-player.js
• add-players.js

Separating them like that helps to reveal some structural issues. For example:

• manage-cover - undeclared videoPlayer
• video-player - undeclared YT
• add-player - undeclared videoPlayer and manageCover
• add-players - undeclared videoPlayer, spinner, and addPlayer

Some of them are best resolved by declaring a global variable at the top of the code, but that’s a last resort. Usually it’s better to find ways to give the desired information instead.

The video-player code is the only one that expects YT. That’s also because when it loads player_api that script creates YT. The approved way according to JSLint is to define a variable:

let YT = undefined;

But, the player_api code refuses to run when it finds that YT already exists, even if it’s undefined, saying: Uncaught SyntaxError: Identifier 'YT' has already been declared

So, a JSLint directive to expect YT as a global variable is required there instead.

/*global YT */
const videoPlayer = (function makeVideoPlayer() {

The other issues are ones that I’ll plug away at in due course.

[Paul_Wilkins]

The manageCover code currently uses videoPlayer. The manageCover code also searched for the videoWrapper. Both of those are much more than the manageCover code ever should know about.

We can use an afterClickCallback function that’s called from the end of the coverClickHandler. The afterClickCallback can be setup from the init code.

manage-cover.js

const manageCover = (function makeManageCover() {
    const config = {
        afterClickCallback: null
    };
...
    function coverClickHandler(evt) {
        ...
        config.afterClickCallback(cover);
    }
...
    function init(cover, afterClickCallback) {
        config.afterClickCallback = afterClickCallback;
        cover.addEventListener("click", coverClickHandler);
    }

We can then pass additional things as that callback to do things with videoPlayer, from the addPlayer code.

add-player.js

    function afterReadyWrapper(cover, afterPlayerReady) {
        function showVideo(cover) {
            const videoWrapper = cover.nextElementSibling;
            manageCover.show(videoWrapper);
            videoPlayer.play(videoWrapper);
        }
        return function playerReadyCallback() {
            manageCover.init(cover, showVideo);
            cover.dispatchEvent(afterPlayerReady);
        };
    }

That clears up the manageCover code, but reveals a new issue. It’s not appropriate to call manageCover.show when dealing with the video. We don’t want to duplicate show and hide on different sets of code either. Instead, show and hide should be available to everything that wants to use it. We can move that into a utils object instead.

utils.js

const utils = (function makeUtils() {
    function hide(el) {
        el.classList.add("hide");
    }
    function show(el) {
        el.classList.remove("hide");
    }
    return {
        hide,
        show
    };
}());

That way, both manageCover and addPlayer can use show and hide from the same place:

manage-cover.js

    function coverClickHandler(evt) {
        const cover = evt.currentTarget;
        utils.hide(cover);
        config.afterClickCallback(cover);
    }

add-player.js

        function showVideo(cover) {
            const videoWrapper = cover.nextElementSibling;
            utils.show(videoWrapper);
            videoPlayer.play(videoWrapper);
        }

The code at https://jsitor.com/1xqkXKIrsv has as usual been updated.

That just leaves the add-player and add-players code to be worked on.

[Paul_Wilkins]

The addPlayer code has a lot of interaction with videoPlayer, so adding that as a global makes good sense.

add-player.js

/*global utils videoPlayer */
const addPlayer = (function makeAddPlayer() {

There are two other things that are referenced though, one being YT and the other being manageCover.

YT is only accessed to get the numeric value of the playing and ended player state. Even though it is possible to not access YT and just put in the appropriate values, and those values aren’t likely to change, but it is more reliable to make use of the PlayerState information that is already there, and it is also appropriate for the addPlaer code to access the PlayerState information. Due to that, adding YT to the list of globals makes good sense.

add-player.js

/*global utils videoPlayer YT */
const addPlayer = (function makeAddPlayer() {

With manageCover though, that’s used when the player is ready.

        return function playerReadyCallback() {
            manageCover.init(cover, showVideo);
            cover.dispatchEvent(afterPlayerReady);
        };

In this case it’s quite inappropriate for the addPlayer code to know anything about manageCover. We should remove that and put it into the afterPlayerReady event instead.

To achieve that we can adjust the afterPlayerReady code to be a function instead, and we want to have cover available, so we need to check evt to find out how we can get cover.

add-players.js

    addPlayer.init({
        afterAddPlayer: toggleSpinner,
        afterPlayerReady: function (evt) {
            toggleSpinner();
            console.log(evt);
        }
    });

Because we are wanting different cover information on the event, we shouldn’t define the event types immediately up front.

add-player.js

    // const events = {
    //     afterAddPlayer: new Event("afterAddPlayer"),
    //     afterPlayerReady: new Event("afterPlayerReady")
    // };
    const events = {};

We can define those events in the addEvents function instead.

    function addEvents(cover) {
        events.afterAddPlayer = new Event("afterAddPlayer");
        events.afterPlayerReady = new Event("afterPlayerReady");
        cover.addEventListener("afterAddPlayer", config.afterAddPlayer);
        cover.addEventListener("afterPlayerReady", config.afterPlayerReady);
    }

Adding the cover information to the events is now easy to achieve, by using CustomEvent instead.

    function addEvents(cover) {
        events.afterAddPlayer = new Event("afterAddPlayer");
        events.afterPlayerReady = new CustomEvent("afterPlayerReady", {
            detail: {cover}
        });
        cover.addEventListener("afterAddPlayer", config.afterAddPlayer);
        cover.addEventListener("afterPlayerReady", config.afterPlayerReady);
    }

With a custom event, all the custom stuff must be accessed via the detail property.

The addPlayers code can now access the cover information.

    addPlayer.init({
        afterAddPlayer: toggleSpinner,
        afterPlayerReady: function (evt) {
            toggleSpinner(evt);
            console.log({cover: evt.detail});
        }
    });

So we can now move the manageCover code out of addPlayer, and in to addPlayers instead.

add-player.js

    function afterReadyWrapper(cover, afterPlayerReady) {
        return function playerReadyCallback() {
            cover.dispatchEvent(afterPlayerReady);
        };
    }

add-players.js

    function showVideo(cover) {
        const videoWrapper = cover.nextElementSibling;
        utils.show(videoWrapper);
        videoPlayer.play(videoWrapper);
    }
    addPlayer.init({
        afterAddPlayer: toggleSpinner,
        afterPlayerReady: function (evt) {
            const cover = evt.detail.cover;
            toggleSpinner(evt);
            manageCover.init(cover, showVideo);
        }
    });

Lastly because it is a CustomEvent being used, we should add that to the list of globals too.

add-player.js

/*global utils videoPlayer YT CustomEvent */
const addPlayer = (function makeAddPlayer() {

And that is the addPlayer code more appropriately structured. The code at https://jsitor.com/1xqkXKIrsv has been updated, and there is only the addPlayers code that’s left to investigate.

[Paul_Wilkins]

The addPlayers code is the last set of code to be investigated. It has a whole lot of things that it uses, including utils, spinner, manageCover, videoPlayer, and addPlayer.

The addPlayers code connects a lot of things together so it’s appropriate for it to reference a lot of things. However, that utils one doesn’t need to be there. It’s more appropriate for the videoPlayer code to take responsibility for showing the player instead.

video-player.js

    function show(videoWrapper) {
        utils.show(videoWrapper);
    }

add-players.js

    function showVideo(cover) {
        const videoWrapper = cover.nextElementSibling;
        videoPlayer.show(videoWrapper);
        videoPlayer.play(videoWrapper);
    }
...
    return {
        ...
        show
    };

The videoPlayer references in the addPlayers code are all suitable too, as the addPlayers code defines default settings for the players, and plays them the when the cover is clicked. Because of that it’s also appropriate for the addPlayers code to reference manageCover.

For the sake of consistency though, because the afterPlayerReady code is a function, I’ll also make the afterAddPlayer code a function too.

    addPlayer.init({
        afterAddPlayer: function (evt) {
            toggleSpinner(evt);
        },
        afterPlayerReady: function (evt) {
            toggleSpinner(evt);
            const cover = evt.detail.cover;
            manageCover.init(cover, showVideo);
        }
    });

The code at https://jsitor.com/1xqkXKIrsv has been updated, and now that the number of interactions between code has been reduced, it will be easier to make further progress with putting together tests for the code.

[asasass]

Does the spinner have to be in the code?

Would it make sense to remove it, or no?

or maybe it is good to keep in as a way of letting you know when they are done loading.

[Paul_Wilkins]

@asasass

Does the spinner have to be in the code?

Would it make sense to remove it, or no?

The spinner provides a valuable service, and that is to explain to people using the page that they aren’t allowed to click the cover until the video player has loaded.

Loading the video always costs time. That time can either occur before the click, or after the click, but it must occur. Doing it before the click results in less annoyance for the person using the page then when it happens afterwards.