Toggling the background color of a page using JavaScript closures

Question

This question is to see if I can get some input on the "design pattern" I tried to implement here. I'm just learning about closure in JavaScript and I think I'm starting to get it. I'm wondering if the way I wrote this code is or is not terrible stylistically.

The code in question is attempting to answer a challenge given in the JavaScript: The Hard Parts (by Will Sentance) on Frontend Masters. Based on the challenge, it seems I went above and beyond as the solution provided, when run, doesn't work properly.

The challenge:

Write a function changeColor that when invoked will first check if the current page background color is "rgb(221, 238, 255)". If it is, it changes the color to "rgb(255, 238, 221)". If it isn't, it sets the color to "rgb(221, 238, 255)".

Add a click event listener to button #1 above (it has an id of "activate"). On click, the button should log to the console "clicked 1". It should also set up a click event listener on button #2 (id of "color"). That listener should log to console "clicked 2" and then call the changeColor function you just created.

Clear the console and hit the 'Run with JS' button. Look at what code has run by analyzing the console. Then try to change the background color by clicking button #2. What needs to happen for the button to work?

Based on the wording of the challenge it seemed like the solution that was hinted at was (in pseudo-ish code):

// Provided solution to the question above 
// Assume all necessary HTML is in place

activationButton.addEventListener("click", () => {
  console.log("clicked activation button")
  colorChangerButton.addEventListener("click", () => {
    console.log("clicked color changer button")
    changeTheColor()
  })
})

The obvious problem with the code above is that a new event listener will be added to the colorChangerButton every time you click the activationButton. This was where I started thinking about trying to use a closure to keep track of a state variable so that I could make sure that the event listener only gets added to the second button once. Also, this is the solution provided in the solution set for the challenge.

To that end, this is what I came up with (please assume that the correct HTML is there, etc):

(function () {

  let clickCount = 0

  function changeColor() {
    if (document.body.style.backgroundColor === "rgb(221, 238, 255)") {
      document.body.style.backgroundColor = "rgb(255, 238, 221)"
    } else {
      document.body.style.backgroundColor = "rgb(221, 238, 255)"
    }
  }

  function activateButton2() {
    const btn2 = document.querySelector('#color') // Appropriate to grab button here?
    btn2.addEventListener("click", () => {
      console.log("clicked #2")
      changeColor()
    })
  }

  const btn1 = document.querySelector('#activate') // Appropriate to have variable declaration here?
  btn1.addEventListener("click", () => {
    clickCount++
    if (clickCount === 1) {
      activateButton2()
    } 
    console.log("clicked #1")
    return
  })
  // Initially had an un-necessary return statement here
})()

The code above - that I wrote - is working how I'd expect it to - the color change functionality and the console logs are firing as I would expect.

One question I had about my implementation was about containing the button variables inside the IIFE. I feel like I see a lot of front-end scripts putting all the query selectors at the top of the script - is there a reason to do that vs. including them within a function like this? If I understand closure correctly, once the event handlers are created there is a persistent reference to everything in the IIFE, so I can fire this function and the buttons and their handlers persist and behave as expected.

It also seems like I could probably use a boolean to track the "clicked" status of the first button, and a switch statement for checking the background color of the page.

Any other pointers anyone wants to offer?

Answer 1

This is a partial answer, that only addresses style, but I prefer something like this for the changeColor function.

I'm not thrilled with activateColorButton, but it works.

This is much more readable and maintainable to me:

const changeColor = e => {
    const lightOrange = "rgb(221, 238, 255)"
    const lightBlue = "rgb(255, 238, 221)"
    const {style} = document.body
    style.backgroundColor = style.backgroundColor === lightOrange 
        ? lightBlue
        : lightOrange
}

const activateColorButton = e => {
    const btnColor = document.querySelector('#color');
    btnColor.removeEventListener('click', changeColor);
    btnColor.addEventListener('click', changeColor);
}

Some Explanation:

TLDR

• Use good variable names
• Avoid duplication
• Use whitespace effectively
• Prepare assignments for future abstraction

/TLDR

In changeColor, I have eliminated as much duplication as possible by assigning variables with names that are deliberately chosen to help the code to describe what it does without additional comments.

Moving the variable names to the top of the function allows them to be extended, changed, or even abstracted out to another function or data structure entirely.

The repeated reference to document.body.style.backgroundColor takes up half of a line of code, is very distracting, hard to read, hard to reason about, and unnecessary. style.backgroundColor could not be reduced to simply backgroundColor. This was as far down as it would go without losing functionality.

I personally find the compactness of the tertiary if statement to be more readable than the explicit if statement and especially prefer it when either assigning or returning one of 2 choices.

I renamed activateButton2 to activateColorButton b/c it more clearly expresses its purpose.

In activateColorButton, I chose to remove the click handler if it's already there and reassign it, rather than have confusing 'flag' variables lying around. I'm not thrilled with it b/c, ideally, it would check to see if the click handler was already assigned via the DOM directly, but I was too lazy to implement that here, which seems ok in this case.