A "C" shape of 7 boxes that turn green on click

Company name asks you to build a website with 7 boxes shaped like a c, which should turn green on click, and then when all are clicked, should turn red in reverse clicking order.

https://jsfiddle.net/yx296/dvacLduw/



document.addEventListener('DOMContentLoaded', function() {
var boxHolder = document.querySelector(".boxHolder");
var count = 0;

boxHolder.addEventListener('click', changeGreen);

var boxes = [];

function changeGreen(e) {
if (e.target !== e.currentTarget) {
var box = e.target;
if (box.style.backgroundColor !== 'green') {
box.style.backgroundColor = 'green';
count++;
boxes.push(e.target);
}
}

if (count === 7) {
count = 0;
setTimeout(function() {
reverseTurnRed();
}, 1000);
}
e.stopPropagation();
}

function reverseTurnRed() {
boxes.reverse();
boxes.forEach(function(box, index) {
setTimeout(function() {
box.style.backgroundColor = 'red';
}, index * 500)
});
setTimeout(function() {
boxes = [];
}, 0)
}
});
div {
box-sizing: border-box;
}

.container {
font-size: 0;
margin: 0 auto;
width: 400px;
height: 400px;
border: 1px solid blue;
position: relative;
}

.boxHolder {
border: 1px solid black;
display: inline-block;
position: absolute;
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
}

.box {
width: 50px;
height: 50px;
border: 1px solid black;
margin: 2px;
}

.topRow {
display: inline-block;
}

.bottomRow {
display: inline-block;
}



vanillaJSAddeparChallenge




















It seems it doesn't matter whether or not I leave the e.stopPropagation in there... what is the point of it?

Willing to accept any critique on HTML, CSS or JavaScript.

This has "interview question" all over it :D Anyways.

First, I'd avoid inline styles. The most common issue with inline styles is that they are hard to override. Only !important can override them from a stylesheet, aside from changing the inline style values themselves. If you carry over the habit to building large apps, CSS maintenance becomes a hell lot worse.

Additionally, styles in JS is hard to maintain. In a large code base, looking for which JS changed some style is hard. You don't know where it happened and the debugger won't tell you immediately without putting DOM breakpoints, which is annoying when a lot of changes take place.

As an alternative, I suggest you define CSS classes corresponding to the state of the box and programmatically add/remove classes. I would suggest reading about BEM, especially modifiers (the "M"). We can use modifiers as "state".

// Base box style
.c-box__box{
  color: transparent
}

// During the "selected" state, you add this class.
.c-box__box--selected{
  color: green
}

// During the "unselected" state, add this class.
.c-box__box--unselected{
  color: red
}

In the sample CSS above, a box would initially be transparent. Clicking the box, you make the box turn to a "selected" state. Thus you add .c-box__box--selected. When you unselect, you make the box turn to an "unselected" state. You remove the .c-box__box--selected and add .c-box__box--unselected. Since styles are defined accordingly in CSS, it will color the box appropriately. Now JS has no idea of styling, only classes pertaining the state of the boxes.

e.stopPropagation() stops the bubbling process, and prevents the event from going up the ancestors and firing events that are attached there. In this case, you don't need it as you are not interested in stopping some event handler in some ancestor from firing.

In your code, you're manually counting your elements (I see a hard 7 there). An alternative would be to use querySelectorAll to get each box and attach a handler. When they get clicked, they get stored in another array. When that array's length matches the length from querySelectorAll, then you can say all your boxes have been selected and you can start your unwinding process. A reverse operation would simply be a pop of the array of selected boxes. This way, you can add an arbitrary amount of boxes without worrying about a hard-coded limit.

Also, instead of a recursive setTimeout, you can use a more loop-like setInterval. When you reach empty, you simply stop the interval from running. Also note that timers are async. One can easily do something between the unwinding process, like say click on a red box. I suggest you put a flag to tell the handler to not proceed while it's unwinding.

Here's a demo (warning: I wrote it using SCSS and ES6): http://codepen.io/anon/pen/JGRzVe



var boxesNodeList = document.querySelectorAll(".c-box__box");
var boxesArray = Array.prototype.slice.call(boxesNodeList);
var selectedBoxes = [];
var isUnwinding = false;

boxesArray.forEach(function(box) {

box.addEventListener('click', function() {
if (isUnwinding) return;

box.classList.remove('c-box__box--unselected');
box.classList.add('c-box__box--selected');
selectedBoxes.push(this);

if (selectedBoxes.length === boxesArray.length) {
isUnwinding = true;
var timer = setInterval(function() {
var box = selectedBoxes.pop();
box.classList.remove('c-box__box--selected');
box.classList.add('c-box__box--unselected');
if (!selectedBoxes.length) {
clearInterval(timer);
isUnwinding = false;
}
}, 500);
}
});
});
.c-box {
display: table;
border: 1px solid blue;
padding: 100px;
overflow: hidden;
}
// Base box style
.c-box__box {
float: left;
border: 1px solid #000;
width: 50px;
height: 50px;
margin: 2px;
// Every 3n+1, break to a new line
&: nth-child(3n + 1) {
clear: both;
}
}
// Just a transparent box
.c-box__spacer {
display: inline-block;
border: transparent;
width: 50px;
height: 50px;
margin: 2px;
}
// Selected state style
.c-box__box--selected {
background: green;
}
// Unselected state style
.c-box__box--unselected {
background: red;
}












StackExchange Code Review Q#115189, answer score: 8