Have you posted a link to the code that results in the above warnings?
It’s not there anymore, if I see it again I will let you know.
This is the code that is being worked on.
https://jsfiddle.net/vy2L4b7x/
Where the playlist is not visible.
I’ve found out how to recreate it, with the following code from https://jsfiddle.net/dqncvkpb/:
const playerVars = Object.assign({},
playerDefaults.playerVars,
settings.playerVars
);
The Object.assign parameters started out being given on the same line, so JSLint expects all of the function parameters to be given on the same line.
const playerVars = Object.assign({}, playerDefaults.playerVars, settings.playerVars);
But that causes a different problem of being too long.
Reducing the size of the variable names is one way to deal with that, but then we lose clarity of what’s going on there.
Breaking down the line to multiple statements is another way to deal with things:
const playerVars = Object.assign({}, playerDefaults.playerVars);
Object.assign(playerVars, settings.playerVars);
But then it’s less clear what is happening, and why.
We could split it again to help make it more clear what is happening:
const playerVars = {};
Object.assign(playerVars, playerDefaults.playerVars);
Object.assign(playerVars, settings.playerVars);
With that, it’s a lot easier to understand what is happening.
But if we’re going to do that then we can just split the original one across multiple lines too:
const playerVars = Object.assign(
{},
playerDefaults.playerVars,
settings.playerVars
);
And that is the final solution that I went with.
The error occurred due to the inconsistent formatting that you changed things to. Fortunately there are other valid ones to pick from.
1 Like
jslint said 0 errors when i put the code in like this.
https://jsfiddle.net/px8jtasg/
(function initCover() {
function hide(el) {
el.classList.add("hide");
}
function coverClickHandler(evt) {
const cover = evt.currentTarget;
hide(cover);
}
const covers = document.querySelectorAll(".jacket");
covers.forEach(function addHandler(cover) {
cover.addEventListener("click", coverClickHandler);
});
}());
const videoPlayer = (function makeVideoPlayer() {
const players = [];
let playerVars = {};
let player = null;
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 shufflePlaylist(player) {
player.setShuffle(true);
player.playVideoAt(0);
player.pauseVideo();
}
function onPlayerReady(event) {
player = event.target;
player.setVolume(100); // percent
shufflePlaylist(player);
}
function onPlayerStateChange(event) {
player = event.target;
if (event.data === YT.PlayerState.PLAYING) {
players.forEach(function pauseOtherVideos(player) {
if (player !== event.target) {
player.pauseVideo();
}
});
}
if (playerVars.loop && event.data === YT.PlayerState.ENDED) {
player.seekTo(playerVars.start);
}
}
function addPlayer(video, settings) {
const playerVarDefaults = {
host: "https://www.youtube-nocookie.com",
videoId: video.dataset.id
};
playerVarDefaults.events = {
"onReady": onPlayerReady,
"onStateChange": onPlayerStateChange
};
playerVars = Object.assign(playerVarDefaults, settings);
players.push(new YT.Player(video, playerVars));
}
return {
addPlayer
};
}());
function loadPlayer(config) {
const playerDefaults = {};
function show(el) {
el.classList.remove("hide");
}
function createPlayerOptions(settings) {
function paramInOptions(opts, param) {
if (settings[param] !== undefined) {
opts[param] = settings[param];
delete settings[param];
}
return opts;
}
const optionParams = ["width", "height", "videoid", "host"];
const preferred = optionParams.reduce(paramInOptions, {});
const playerOptions = Object.assign({}, playerDefaults, preferred);
const playerVars = Object.assign(
{},
playerDefaults.playerVars,
settings.playerVars
);
playerOptions.playerVars = playerVars;
return playerOptions;
}
function initPlayerDefaults(defaults) {
Object.assign(playerDefaults, defaults);
}
function initPlayer(videoWrapper, settings = {}) {
const video = videoWrapper.querySelector(".video");
const playerOptions = createPlayerOptions(settings);
const player = videoPlayer.addPlayer(video, playerOptions);
videoWrapper.player = player;
}
function coverClickHandler(evt) {
const wrapper = evt.currentTarget.nextElementSibling;
show(wrapper);
initPlayer(wrapper, config);
}
const defaults = {
height: 207,
width: 277
};
defaults.playerVars = {
autoplay: 0,
controls: 1,
disablekb: 1,
enablejsapi: 1,
fs: 0,
iv_load_policy: 3,
rel: 0
};
initPlayerDefaults(defaults);
const cover = document.querySelector(config.target);
delete config.target;
cover.addEventListener("click", coverClickHandler);
}
function onYouTubeIframeAPIReady() {
loadPlayer({
height: 207,
playlist: "0dgNc5S8cLI,mnfmQe8Mv1g,-Xgi_way56U,CHahce95B1g",
target: ".jacket-left",
width: 277
});
loadPlayer({
height: 207,
start: 4,
target: ".jacket-middle",
width: 277
});
loadPlayer({
height: 207,
target: ".jacket-right",
width: 277
});
}
Yes that’s right. The following results in zero problems.
const playerVars = Object.assign(
{},
playerDefaults.playerVars,
settings.playerVars
);
Moving the empty object up to the previous line creates those problems:
const playerVars = Object.assign({},
playerDefaults.playerVars,
settings.playerVars
);
1 Like
ok, so then it is fixed.
how does the playlist issue get fixed?
https://jsfiddle.net/px8jtasg/
Post 3 is where the playlist code worked last.
Well lets follow it through.
loadPlayer({
height: 207,
playlist: "0dgNc5S8cLI,mnfmQe8Mv1g,-Xgi_way56U,CHahce95B1g",
target: ".jacket-left",
width: 277
});
It’s received by loadPlayer as config:
function loadPlayer(config) {
and given to initPlayer in the coverClickHandler
function coverClickHandler(evt) {
const wrapper = evt.currentTarget.nextElementSibling;
show(wrapper);
initPlayer(wrapper, config);
}
Because the loadPlayer code removes the cover property from config, it now only contains what should be the player options and playerVars, so the initPlayer function receives it as settings which combines both the options and playerVars:
function initPlayer(videoWrapper, settings = {}) {
and those settings in there are given to createPlayerOptions:
const playerOptions = createPlayerOptions(settings);
playlist still exists in settings, where the player options and playerVars are still mixed together.
The createPlayerOptions function receives the settings:
function createPlayerOptions(settings) {
and it’s in here that the playerVars are assigned:
const playerVars = Object.assign(
{},
playerDefaults.playerVars,
settings.playerVars
);
playerOptions.playerVars = playerVars;
It’s in there that the problem occurs.
If we had used tests to develop the code, the problem would have been instantly reported to us.
But as things are without the tests, it’s hard to tell what the problem is.
I’ve given all of the information in the above code excerpts.
Can you tell what the problem is yet?
The problem comes from that the settings parameter doesn’t have playerVars as a property. The settings parameter instead smushes the options and playerVars together for convenience.
Because of that, there is no playerVars to get from settings.
const playerVars = Object.assign(
{},
playerDefaults.playerVars,
settings.playerVars
);
Instead of settings.playerVars we should just use settings.
const playerVars = Object.assign(
{},
playerDefaults.playerVars,
settings
);
We might even bring that back up to a single line:
const playerVars = Object.assign({}, playerDefaults.playerVars, settings);
But that’s still considered to be too long. We can solve that by renaming playerDefaults to be just defaults. That means adjusting a few other things in the code, but we are left with the following:
const playerVars = Object.assign({}, defaults.playerVars, settings);
And just to try and ensure that the same problem doesn’t happen again, a comment can be left about the changing nature of the settings parameter.
// settings should now only consist of playerVars
const playerVars = Object.assign({}, defaults.playerVars, settings);
The updated code is found at: https://jsfiddle.net/quabrytc/
1 Like
Should these be reversed?
function loadPlayer(config) {
const defaults = {
height: 207,
width: 277
};
defaults.playerVars = {
autoplay: 0,
controls: 1,
disablekb: 1,
enablejsapi: 1,
fs: 0,
iv_load_policy: 3,
rel: 0
};
function show(el) {
el.classList.remove("hide");
}
This instead?
function loadPlayer(config) {
function show(el) {
el.classList.remove("hide");
}
const defaults = {
height: 207,
width: 277
};
defaults.playerVars = {
autoplay: 0,
controls: 1,
disablekb: 1,
enablejsapi: 1,
fs: 0,
iv_load_policy: 3,
rel: 0
};
It’s best to have defaults like that either at the top of the function or the bottom. Either of those places are easier to find than somewhere in the middle of it.
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/
1 Like
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.
1 Like
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.
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.
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.
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.
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.
2 Likes
This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.