I don’t think so.
What do they do?
Magic values are a problem, they are an extremely specific thing and those cause problems.
What we did with “.thePlay” is how to fix the problem, by moving those magic values out.
There is only one other magic value in the manageCover code and that is “.container”. Shall we deal with that now?
1 Like
Yes.
Here is the problem function with its magic value.
function hideContainers() {
const allContainers = document.querySelectorAll(".container");
allContainers.forEach(hide);
}
We can use the function parameter to move the problem elsewhere, out of this function.
function hideContainers(containers) {
allContainers.forEach(hide);
}
...
function coverClickHandler(evt) {
const containers = document.querySelectorAll(".container");
hideContainers(containers);
showCovers(evt.currentTarget);
}
That hideContainers function now doesn’t need to be restricted to containers. It can become a more generic hideAll function instead.
function hideAll(elements) {
elements.forEach(hide);
}
...
function coverClickHandler(evt) {
const containers = document.querySelectorAll(".container");
hideAll(containers);
...
}
We still have “.container” as a problem magic string in the coverClickHandler function.
There’s no good way for the coverClickHandler code to get “.container” from the event object, so that information should come from somewhere else.
This is where as add a config object at the top of the code. The magic string can sit up there temporarily for now.
const manageCover = (function makeManageCover() {
const config = {
containers: document.querySelectorAll(".container")
}
...
function coverClickHandler(evt) {
hideAll(config.containers);
showCovers(evt.currentTarget);
}
That coverClickHandler code is much improved now.
That magic string is still in the code though up in the config section. Ideally we want to use the initialize code to specify the container selector, so lets move that magic string to the init code instead.
const manageCover = (function makeManageCover() {
const config = {};
...
function init(playButtonSelector) {
config.containers = document.querySelectorAll(".container");
const playButtons = document.querySelectorAll(playButtonSelector);
addClickToButtons(playButtons);
}
We can now use init with a selectors object, and get the strings from there.
function init(selectors) {
config.containers = document.querySelectorAll(selectors.container);
const playButtons = document.querySelectorAll(selectors.playButton);
addClickToButtons(playButtons);
}
...
manageCover.init({
container: ".container",
playButton: ".thePlay"
});
That is how magic values are extracted from the code, so that they can be more beneficially and reliably defined in one place.
We moved the problem magic string
- out of its function to the event handler that called it
- out of the event handler up to a config variable
- out of the config to be defined in the init function
- out of the init function to be defined when intializing the code
Each part of the journey was easy to do, and results in even more improvement each time.
1 Like
I am still working on getting it working.
The browser console says “addClickToButtons is not defined”
It looks like you accidentally removed that whole function.
This isn’t highlighted
hideContainers(config.containers);
https://jsfiddle.net/7kq93tjp/2/
I was supposed to remove this right?
function hideContainers() {
const allContainers = document.querySelectorAll(".container");
allContainers.forEach(hide);
}
and replace it with this:
function hideAll(elements) {
elements.forEach(hide);
}
Yes, it was transformed into the more generic and useful hideAll function instead.
What do I do about hideContainers?
https://jsfiddle.net/ow9p40L8/1/
I deleted this:
function hideContainers() {
const allContainers = document.querySelectorAll(".container");
allContainers.forEach(hide);
}
The code isn’t working.
1 Like
The hideContainers in the coverClickListener should be hideAll instead.
1 Like
A further improvement is in regard to the separate cover code near the end of the code.
function coverClickHandler(evt) {
const wrapper = evt.currentTarget.nextElementSibling;
show(wrapper);
initPlayer(wrapper, config);
}
That shouldn’t be separate from the other cover code. The manageCover code can be updated so when you use manageCover.init(…), that other stuff can be included too.
What order should these be in?
function coverClickHandler(evt) {
const wrapper = evt.currentTarget.nextElementSibling;
hideAll(config.containers);
showCovers(evt.currentTarget);
show(wrapper);
initPlayer(wrapper, config);
}
No that’s quite the wrong thing to do. The manageCover code should have absolutely no knowledge of other things like initPlayer and wrapper. It’s through manageCover.init(…) that we can introduce those things instead.
function coverClickHandler(evt) {
const wrapper = evt.currentTarget.nextElementSibling;
show(wrapper);
initPlayer(wrapper, config);
}
What do I do about this one?
The stuff inside it?
function coverClickHandler(evt) {
hideAll(config.containers);
showCovers(evt.currentTarget);
}
This?
function coverClickHandler(evt) {
const wrapper = evt.currentTarget.nextElementSibling;
show(wrapper);
initPlayer(wrapper, config);
}
function addClickToButtons(playButtons) {
playButtons.forEach(function addEventHandler(playButton) {
playButton.addEventListener("click", coverClickHandler);
});
}
function init(selectors) {
config.containers = document.querySelectorAll(selectors.container);
const playButtons = document.querySelectorAll(selectors.playButton);
addClickToButtons(playButtons);
}
return {
init
};
}());
manageCover.init({
container: ".container",
playButton: ".thePlay"
});
Am I adding to this?
https://jsfiddle.net/1sn35t9u/
manageCover.init({
container: ".container",
playButton: ".thePlay"
});
How do I add:
hideAll(config.containers);
showCovers(evt.currentTarget);
To:
manageCover.init({
If that is what I am supposed to be doing.
It looks like you are stomping around destroying all of the good work.
I’ll start from the last working version here:
https://jsfiddle.net/ow9p40L8/3/
This:
function coverClickHandler(evt) {
const wrapper = evt.currentTarget.nextElementSibling;
show(wrapper);
initPlayer(wrapper, config);
}
Replaces
function coverClickHandler(evt) {
hideAll(config.containers);
showCovers(evt.currentTarget);
}