It’s working here:
It was missing:
}());
https://jsfiddle.net/zqpse9kd/2/
function coverClickHandler(evt) {
hideContainers();
showCovers(evt.currentTarget);
}
addClickToButtons();
}());
1 Like
I think this is the right order:
https://jsfiddle.net/zqpse9kd/4/
function hideContainers() {
const allContainers = document.querySelectorAll(".container");
allContainers.forEach(function hideContainer(container) {
container.classList.add("hide");
});
}
function showCovers(playButton) {
const cover = playButton.parentElement;
cover.classList.add("active");
cover.classList.remove("hide");
}
function coverClickHandler(evt) {
hideContainers();
showCovers(evt.currentTarget);
}
function addClickToButtons() {
const allPlayButtons = document.querySelectorAll(".thePlay");
allPlayButtons.forEach(function addEventHandler(playButton) {
playButton.addEventListener("click", coverClickHandler);
});
}
addClickToButtons();
}());
Also, these aren’t needed at the moment right now.
function show(el) {
el.classList.remove("hide");
}
function hide(el) {
el.classList.add("hide");
}
Those are useful. You should replace:
container.classList.add("hide");
with:
hide(container);
and replace:
cover.classList.remove("hide");
with
show(cover);
That way the details of showing and hiding things remains only in the one place in those functions.
1 Like
I did that here:
https://jsfiddle.net/zqpse9kd/7/
function manageCover() {
function show(el) {
el.classList.remove("hide");
}
function hide(el) {
el.classList.add("hide");
}
function hideContainers() {
const allContainers = document.querySelectorAll(".container");
allContainers.forEach(function hideContainer(container) {
hide(container);
});
}
function showCovers(playButton) {
const cover = playButton.parentElement;
cover.classList.add("active");
show(cover);
}
function coverClickHandler(evt) {
hideContainers();
showCovers(evt.currentTarget);
}
function addClickToButtons() {
const allPlayButtons = document.querySelectorAll(".thePlay");
allPlayButtons.forEach(function addEventHandler(playButton) {
playButton.addEventListener("click", coverClickHandler);
});
}
addClickToButtons();
}());
The hideContainers function can now be simplified too. Here is is beforehand.
function hideContainers() {
const allContainers = document.querySelectorAll(".container");
allContainers.forEach(function hideContainer(container) {
hide(container);
});
}
Because the container variable from hideContainer is being passed directly to hide, we can just use forEach with hide.
function hideContainers() {
const allContainers = document.querySelectorAll(".container");
allContainers.forEach(hide);
}
1 Like
I did that here:
https://jsfiddle.net/s0g2e98f/1/
function hideContainers() {
const allContainers = document.querySelectorAll(".container");
allContainers.forEach(hide);
}
function showCovers(playButton) {
const cover = playButton.parentElement;
cover.classList.add("active");
show(cover);
}
function coverClickHandler(evt) {
hideContainers();
showCovers(evt.currentTarget);
}
function addClickToButtons() {
const allPlayButtons = document.querySelectorAll(".thePlay");
allPlayButtons.forEach(function addEventHandler(playButton) {
playButton.addEventListener("click", coverClickHandler);
});
}
addClickToButtons();
}());
Good one. Now there are other improvements that can be made, but those are about making future work with the code easier to achieve.
Improvements like what, what else would need to be done?
Right now the code automatically runs and can’t be configured for different situations.
The code automatically running is a problem because that can’t be adjusted in any way without fiddling with the code. It’s important that defining what the mangeCover code does, can be separated from when it does it. That is achieved by using an init function so that outside of the manageCover code, you can run that init function to get things going.
That init function also helps with configuring for different situations. The selector “.thePlay” can be given when we initialize the manageCover code, and that selector can then be used to get all of the play buttons.
1 Like
I can continue with improvements now, if not now it can wait for another time. And thank you for your help with this.
To make the manageCover code available for innitializing later on, we need to assign it to a local variable.
const manageCover = (function makeManageCover() {
Right now that manageCover is undefined, so we return an object from the end of the function:
return {
};
}());
And in that object we return anything that we want to make available. That effectively gives us a private/public separation where everything inside of the code is private, and whatever is returned from that object is public.
The addClickToButtons() function is what we want to happen when we start the code, so put that in the init function.
function init() {
addClickToButtons();
}
We want to make that init function public so that we can initialize manageCover, so put init in the returned object.
return {
init
};
}());
There is now a clear separation between what the code does, and when it does it. We use manageCover.init() to get things going. Usually it’s at the end of the code when that is done, so that all of the functions are above, and then what happens with them is grouped together right at the end.
manageCover.init();
1 Like
That’s much better:
https://jsfiddle.net/64oqwy2h/1/
const manageCover = (function makeManageCover() {
function show(el) {
el.classList.remove("hide");
}
function hide(el) {
el.classList.add("hide");
}
function hideContainers() {
const allContainers = document.querySelectorAll(".container");
allContainers.forEach(hide);
}
function showCovers(playButton) {
const cover = playButton.parentElement;
cover.classList.add("active");
show(cover);
}
function coverClickHandler(evt) {
hideContainers();
showCovers(evt.currentTarget);
}
function addClickToButtons() {
const allPlayButtons = document.querySelectorAll(".thePlay");
allPlayButtons.forEach(function addEventHandler(playButton) {
playButton.addEventListener("click", coverClickHandler);
});
}
function init() {
addClickToButtons();
}
return {
init
};
}());
manageCover.init();
Now with the init function we can accept a playButton selector, and use that to get the play buttons.
function init(playButtonSelector) {
const playButtons = document.querySelectorAll(".thePlay");
addClickToButtons(playButtons);
}
That lets us initialize things with those play buttons.
manageCover.init(".thePlay");
The addClickToButtons function can now be adjusted to more properly work using those playButtons.
function addClickToButtons(playButtons) {
playButtons.forEach(function addEventHandler(playButton) {
playButton.addEventListener("click", coverClickHandler);
});
}
The addClickToButtons function has now gone from being very specific, to more generic.
Code goes on a journey from more specific to generic as it improves to handle different situations,
Achieving that specific to generic improvement can be challenging, but is very rewarding.
This is being deleted?
function init() {
addClickToButtons();
}
Not deleted - replaced.
Like this?
function coverClickHandler(evt) {
hideContainers();
showCovers(evt.currentTarget);
}
function addClickToButtons(playButtons) {
playButtons.forEach(function addEventHandler(playButton) {
playButton.addEventListener("click", coverClickHandler);
});
}
function init(playButtonSelector) {
const playButtons = document.querySelectorAll(".thePlay");
addClickToButtons(playButtons);
}
return {
init
};
}());
manageCover.init(".thePlay");
Other than some formatting to tidy it, it seems okay.
Have you heard about magic values, such as magic strings or magic numbers?