8
\$\begingroup\$

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;
}
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>vanillaJSAddeparChallenge</title>
  <link rel="stylesheet" href="styles.css">
</head>
<body>
  <div class="container">
    <div class="boxHolder">
      <div class="topRow box"></div>
      <div class="topRow box"></div>
      <div class="topRow box"></div>
      <div class="box"></div>
      <div class="bottomRow box"></div>
      <div class="bottomRow box"></div>
      <div class="bottomRow box"></div>
    </div>
  </div>

  <script src="practice.js"></script>
</body>
</html>

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.

\$\endgroup\$
0

2 Answers 2

Reset to default
8
\$\begingroup\$

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;
}
<div class="c-box">
  <div class="c-box__box"></div>
  <div class="c-box__box"></div>
  <div class="c-box__box"></div>
  <div class="c-box__box"></div>
  <div class="c-box__spacer"></div>
  <div class="c-box__spacer"></div>
  <div class="c-box__box"></div>
  <div class="c-box__box"></div>
  <div class="c-box__box"></div>
</div>

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I like the advice about adding classes instead of doing inline styles with Javascript---- however I did notice in your fiddle if you click on 6 boxes and then click a box you already clicked instead of the 7th unclicked box they will start turning red--- is that intentional? \$\endgroup\$
    – WinchenzoMagnifico
    Commented Dec 28, 2015 at 4:58
  • \$\begingroup\$ @WinchenzoMagnifico It's a flaw in my logic. I didn't check if the box was already selected (equivalent to your !== 'green'). You can use indexOf to check if something is already in the array. \$\endgroup\$
    – Joseph
    Commented Dec 28, 2015 at 12:45
7
\$\begingroup\$

Throwing in the green flag

if (box.style.backgroundColor !== 'green') {

This string comparison isn't very efficient. By the looks of it, you are trying to use this property as a flag.

Instead, you should use an actual flag: set a boolean data attribute on the box to tell you in the future what state the element is in.

I can count!

The variable count isn't needed in this code; it is simply the .length of the boxes array. Here is an example:

if (count === 7) {

is

if (boxes.length == 7) {

Reverse loop

boxes.reverse();
boxes.forEach(function(box, index) {

Here you are creating an entirely new array only to loop over it normally in the next line. To speed up your code, you could simply iterate over the array backwards with a normal for loop:

for(var i = boxes.length; i >= 0; i--) {

Time your threads

boxes.forEach(function(box, index) {
  setTimeout(function() {
    box.style.backgroundColor = 'red';
  }, index * 500)
});

Imagine something on the user's end starts to hang up. This could result in some threads slowing down for a few seconds. That could create this weird effect of some boxes disappearing immediately after another box (or even before them).

To fix this, you should only spawn another thread after one has finished. So, at the end of the first setTimeout callback, you would start the next setTimeout.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.