Setting up single-player tests before adding spinner

Paul_Wilkins · July 20, 2022, 7:17pm

Go back to the previously working code, and I’ll work on giving you better instructions.

asasass · July 20, 2022, 7:20pm

I must’ve got stuck somewhere before.

Working Code: https://jsfiddle.net/tub0mhs1/1/

 function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100);
    if (Array.isArray(config.videoIds)) {
      shufflePlaylist(player);
    }
    const iframe = getIframe(player);
    iframe.dispatchEvent(new Event("afterPlayerReady"));
  }

I guess this is wrong then.

  function isPlaylist() {
    if (isPlaylist(config.videoIds)) {
      shufflePlaylist(config.player);
    }
  }

Paul_Wilkins · July 20, 2022, 8:29pm

Yes, that’s wrong.

The only part of the if statement that needs to change is the condition. The condition of the if statement, is the following highlighted piece:

It is that highlighted section, and only that highlighted section, that gets extracted out to a separate function.

asasass · July 20, 2022, 8:52pm

I have this now. https://jsfiddle.net/hm248nrL/3/

  function isPlaylist() {
    if (Array.isArray(config.videoIds)) {
      shufflePlaylist(config.player);
    }
  }
  
  function shufflePlaylist() {
    config.player.setShuffle(true);
    config.player.playVideoAt(0);
    config.player.stopVideo();
  }
  
  function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100);
    isPlaylist();
    const iframe = getIframe(player);
    iframe.dispatchEvent(new Event("afterPlayerReady"));
  }

Paul_Wilkins · July 20, 2022, 10:33pm

That is not correct. You extracted out much more than the shown highlighted section.

asasass · July 20, 2022, 10:34pm

Is the test supposed to pass or fail, or is that not important?

Are you able to see where I am perhaps getting confused, or having a hard time figuring this out?

I’m lost.

What do I do after I do this?

  function isPlaylist() {
  Array.isArray(config.videoIds)
  }

I have no idea what I am supposed to do after that.

After I extract that I am left with this:

    if () {
      shufflePlaylist(player);
    }

 function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100);
    isPlaylist();
    if () {
      shufflePlaylist(player);
    }
    const iframe = getIframe(player);
    iframe.dispatchEvent(new Event("afterPlayerReady"));
  }

Do I next delete the if statement?

function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100);
    isPlaylist();
    shufflePlaylist(player);
    const iframe = getIframe(player);
    iframe.dispatchEvent(new Event("afterPlayerReady"));
  }

https://jsfiddle.net/ftorcxm5/1/

Expected spy setShuffle-handler not to have been called.

 function isPlaylist() {
    Array.isArray(config.videoIds)
  }

  function shufflePlaylist() {
    config.player.setShuffle(true);
    config.player.playVideoAt(0);
    config.player.stopVideo();
  }

  function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100);
    isPlaylist();
    shufflePlaylist(player);
    const iframe = getIframe(player);
    iframe.dispatchEvent(new Event("afterPlayerReady"));
  }

Paul_Wilkins · July 21, 2022, 12:33am

The test is supposed to remain passing.

You were supposed to extract only the following:

Array.isArray(config.videoids)

to a separate function, but you ended up doing much more than that.

asasass · July 21, 2022, 1:54am

I still do not know what I am doing wrong.

So I am not able to fix it if I don’t know what is wrong.

2nd attempt: https://jsfiddle.net/067tj1oe/2/

Still not passing.

  function isPlaylist() {
    Array.isArray(config.videoIds)
  }

  function shufflePlaylist() {
    config.player.setShuffle(true);
    config.player.playVideoAt(0);
    config.player.stopVideo();
  }

  function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100);
    isPlaylist();
    if (isPlaylist()) {
      shufflePlaylist(player);
    }
    const iframe = getIframe(player);
    iframe.dispatchEvent(new Event("afterPlayerReady"));
  }

Paul_Wilkins · July 21, 2022, 3:50am

The isPlaylist() function needs to return something, and in the onPlayerReady() function you are calling isPlaylist() twice. it should only be called once inside of the condition section of the if statement.

Extracting code is something that modern web browsers make as easy to do as select text, right-click and select Extract. I suspect though that you do not enjoy those benefits, so the below details are how to do it manually instead.

Here’s the process of extracting something out to a separate function:

Create the separate function and give it a name
Return something from that separate function
Ensure that any variables needed by that code are given as function parameters
Call that separate function with any needed arguments.

Here is how it is done.

Create the separate function and give it a name

function isPlaylist() {
}
...
    if (Array.isArray(config.videoIds)) {
      shufflePlaylist(player);
    }

Return something from that separate function

function isPlaylist() {
  return Array.isArray(config.videoIds);
}
...
    if (Array.isArray(config.videoIds)) {
      shufflePlaylist(player);
    }

Ensure that any variables needed by that code are given as function parameters

None are needed here, so we can leave the code as it was.

Call that separate function with any needed arguments.

No function arguments are needed here.

function isPlaylist() {
  return Array.isArray(config.videoIds);
}
...
    if (isPlaylist()) {
      shufflePlaylist(player);
    }

asasass · July 21, 2022, 7:34am

It now passes: https://jsfiddle.net/eu2zckLs/2/

Is more refactoring needed?

Are we able to work on this next?

addPlayer() function to have just a single videoIds parameter, so that regardless of it being undefined, a single video id, an empty array, or a full array of video ids, it will be capable of properly handing everything.

function isPlaylist() {
    return Array.isArray(config.videoIds);
  }

  function shufflePlaylist() {
    config.player.setShuffle(true);
    config.player.playVideoAt(0);
    config.player.stopVideo();
  }

  function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100);
    if (isPlaylist()) {
      shufflePlaylist(player);
    }
    const iframe = getIframe(player);
    iframe.dispatchEvent(new Event("afterPlayerReady"));
  }

Paul_Wilkins · July 21, 2022, 8:15am

The videoPlayer code has commented-out lines of code. Those lines need to be removed.

The createOptions() function currently takes two similar parameters, of videoId and of videoIds. We can use only the one parameter for that, but it does mean first removing the need for videoId.

The first step towards that is for the videoIds parameter to be either an array of ids, or a single video id. We can achieve that in the addPlayer code on the createOptions line by replacing videoIds with videoIds || videoId

That will cause some of the code to break, but we can use our newly created isPlaylist() function in the createOptions() function, by using isPlaylist(videoIds) in the condition of the if statement.

asasass · July 21, 2022, 8:25am

It is still failing.

https://jsfiddle.net/noL65ech/

  function onPlayerReady(event) {
    const player = event.target;
    player.setVolume(100);
    if (isPlaylist(videoId)) {
      shufflePlaylist(player);
    }
    const iframe = getIframe(player);
    iframe.dispatchEvent(new Event("afterPlayerReady"));
  }

  function addPlayer(video, videoIds) {
    checksVideo(video);

    const videoId = video.dataset.id;

    if (!videoId && !videoIds.length) {
      throw new TypeError("A video id is needed.");
    }
    
    const options = createOptions(videoIds || videoId);
    config.player = new YT.Player(video, options);

    const iframe = getIframe(config.player);
    const eventHandler = config.eventHandlers.afterPlayerReady;
    iframe.addEventListener("afterPlayerReady", eventHandler);
    return config.player;
  }

Paul_Wilkins · July 21, 2022, 11:17am

That’s only because you failed to follow instructions.

Go back to working code, and I’ll take you through what needs to be done more slowly, and with more explanation so that you might hopefully gain a better understanding about what gets done and why.

asasass · July 21, 2022, 12:43pm

I figured it out: https://jsfiddle.net/9dckgs5o/2/

What is next to do?

The createOptions() function currently takes two similar parameters, of videoId and of videoIds. We can use only the one parameter for that

That means delete: videoIds

from here: function createOptions(videoId, videoIds) {

in the addPlayer code on the createOptions line by replacing videoIds with videoIds || videoId

That means do this: const options = createOptions(videoIds || videoId);

in the createOptions() function, by using isPlaylist(videoIds) in the condition of the if statement.

That means do this:

    if (isPlaylist(videoIds)) {
      options.playerVars.playlist = createPlaylist(videoIds);
    } else {
      options.videoId = videoIds;
    }
    return options;
  }

Paul_Wilkins · July 21, 2022, 7:15pm

Ahh good you got there. Next up we reduce the amount we rely on config.videoIds. Functions should rely on info from their parameters. The more we breach that and reach out to different things outside of the function, the more unreliable the code becomes. The isPlaylist() function definitely breaches that so here’s what you do:

Replace config.videoIds with only videoIds
That variable doesn’t exist yet so add it to the parameters of the isPlaylist() function.
That parameter is given as a function call argument so each place that isPlaylist is called, use videoIds as the function argument.
You should then find that it’s only in the onPlayerReady() function where videoIds can’t be found, so that is where you use config.videoIds instead.

asasass · July 21, 2022, 8:33pm

I did that here: https://jsfiddle.net/82x3gvfe/

What comes next?

Paul_Wilkins · July 21, 2022, 10:27pm

In the onPlayerReady() function we make it crystal clear that config.videoIds is a weird thing that we are getting info from.

To achieve that we assign at the top of the function a local variable called videoIds, to be config.videoIds

Then throughout the rest of that function we use just videoIds.

That way, the “weird configuration stuff” is easily found at the top of the function, instead of being hidden deep within it.

asasass · July 21, 2022, 10:31pm

Like this? https://jsfiddle.net/whkx1rpm/1/

  function onPlayerReady(event) {
    const videoIds = config.videoIds;
    const player = event.target;
    player.setVolume(100);
    if (isPlaylist(videoIds)) {
      shufflePlaylist(player);
    }
    const iframe = getIframe(player);
    iframe.dispatchEvent(new Event("afterPlayerReady"));
  }

Should we do that here also, or no? https://jsfiddle.net/whkx1rpm/2/

 function onYouTubeIframeAPIReady() {
    const videoIds = config.videoIds;
    const cover = document.querySelector(".play");
    const wrapper = cover.parentElement;
    const frameContainer = wrapper.querySelector(".video");
    config.player = addPlayer(frameContainer, videoIds);
  }

Will we be doing something similar with config.player? https://jsfiddle.net/whkx1rpm/6/

Only this one uses let, the rest use const?

 function onYouTubeIframeAPIReady() {
    let player = config.player;
    const videoIds = config.videoIds;
    const cover = document.querySelector(".play");
    const wrapper = cover.parentElement;
    const frameContainer = wrapper.querySelector(".video");
    player = addPlayer(frameContainer, videoIds);
  }

  function getIframe() {
    const player = config.player;
    return Object.values(player).find(
      (item) => item.nodeName === "IFRAME"
    );
  }

  function play() {
  const player = config.player;
    if (player && player.playVideo) {
      player.playVideo();
    }
  }

I got stuck on how to remove config.player from these:

Both these:
const player = config.player;
let player = config.player;

Don’t work in here.

or I’m doing it wrong, or, it’s not supposed to be done in there.

function addPlayer(video, videoIds) {
    checksVideo(video);

    const videoId = video.dataset.id;

    if (!videoId && !videoIds.length) {
      throw new TypeError("A video id is needed.");
    }

    const options = createOptions(videoIds || videoId);
    config.player = new YT.Player(video, options);

    const iframe = getIframe(config.player);
    const eventHandler = config.eventHandlers.afterPlayerReady;
    iframe.addEventListener("afterPlayerReady", eventHandler);
    return config.player;
  }

Paul_Wilkins · July 22, 2022, 12:46am

No we don’t. That’s because we aren’t getting a value from that external source, we are instead assigning a value to it and that assignment must occur external to the function. Using let or const stops that from happening. so put it back to how it was.

asasass · July 22, 2022, 2:28am

This one is good then? https://jsfiddle.net/whkx1rpm/2/

 function onYouTubeIframeAPIReady() {
    const videoIds = config.videoIds;
    const cover = document.querySelector(".play");
    const wrapper = cover.parentElement;
    const frameContainer = wrapper.querySelector(".video");
    config.player = addPlayer(frameContainer, videoIds);
  }

In regards to config.player; that is not being changed in the code if I understand correctly?

config.player; stays as config.player;

Meaning, we are not doing this:

  function play() {
  const player = config.player;
    if (player && player.playVideo) {
      player.playVideo();
    }
  }

Topic		Replies	Views
Questions about the spinner code JavaScript	42	3363	June 16, 2022
Adding tests to video player code JavaScript	19	4037	December 5, 2021
I'm trying to get the 'player_api' script to work with a single player JavaScript	37	844	March 4, 2019
Should pointer-events: initial; be used on the spinner? HTML & CSS	2	811	July 28, 2022
Converting players to initButton JavaScript	8	1324	January 16, 2019

Setting up single-player tests before adding spinner

Related topics