0

My Program
An Etch-A-Sketch that initially draws on the grid in black. The user clicks Erase to color over the black squares. The user can also click **Rainbow" to draw a random color on each square, i.e. square[0] can be blue, square[1] can be purple...etc, they're randomized on each mouseover.

Problem You'll notice that the code for eraseGrid() and drawRainbow() are nearly identical. I had to do this or the program wouldn't work properly. This is the only way for Rainbow to draw different colors on each mouseover.

Goal If you look at the function I commented out at the bottom, I tried coming up with something I could use for both eraseGrid() and drawRainbow(), but when testing the function out, it wasn't working as intended. Instead of drawing a random color on each mouseover, it would create a random color (let's say blue) and draw blue on the grids. If I toggled Rainbow back on, it would create another random color (green for example) and draw that on the grids.
I don't understand why the function I created doesn't work as intended, whereas the repeated code does.


/**************************** Input->Button DOM ****************************/

const newGrid = document.getElementById('new-grid');
newGrid.addEventListener('click', createGrid);

const erase = document.getElementById('erase');
erase.addEventListener('click', eraseGrid);

const rainbow = document.getElementById('rainbow');
rainbow.addEventListener('click', drawRainbow);

/*********************** Grid variable and creation ***********************/

const main = document.querySelector('main');
const div = main.getElementsByTagName('div');

drawGrid(16, ((600 / 16) - 2) + 'px');
pickColor('#333');

function createGrid() {

    // removes divs from largest to smallest
    for (let i = main.childNodes.length - 1; i >= 0 ; i--) {
        main.removeChild(main.childNodes[i]);
    }

    let size;
    do {
        size = parseInt(prompt("Please enter a number from 1 to 64", ""), 10);
    } while(Number.isNaN(size) || size > 64 || size < 1);

    const numPx = (600 / size) - 2;
    let px = numPx + 'px';

    drawGrid(size, px);
    pickColor('#333');
}


function pickColor(color) {
    for (let i = 0; i < main.childNodes.length; i++) {
        main.childNodes[i].addEventListener('mouseover', function change() {
            main.childNodes[i].style.backgroundColor = color;
        })
    }
}

// draw grid of div elements
function drawGrid(size, px) {
    for (let i = 0; i < size; i++) {
        for (let j = 0; j < size; j++) {
            const div = document.createElement('div');
            main.appendChild(div);
            div.setAttribute('style', `width: ${px}; height: ${px}; 
                float: left; border: 1px solid #333;`);
        }
    }

    // clear floats
    const div = document.createElement('div');
    div.setAttribute('class', 'clear');
    main.appendChild(div);
}

function eraseGrid() {
    erase.classList.toggle('erase');
    if (erase.className === 'erase') {
        rainbow.classList.remove('rainbow');
        main.addEventListener('mouseover', function(){
            pickColor('#f2f2f2');
        })
    }
    else {
        main.addEventListener('mouseover', function(){
            pickColor('#333');
        })
    }
}

function randColor() {
    let arr = [];
    for (let i = 0; i < 3; i++) {
        arr.push(Math.floor(Math.random() * 255));
    }
    return arr;
}

function drawRainbow() {
    rainbow.classList.toggle('rainbow');
    if (rainbow.className === 'rainbow') {
        erase.classList.remove('erase');
        main.addEventListener('mouseover', function(){
            pickColor('rgb(' + randColor() + ')');
        })
    }
    else {
        main.addEventListener('mouseover', function(){
            pickColor('#333');
        })
    }
    // changeColor(rainbow, 'rainbow', erase, 'erase', 'rgb(' + randColor() + ')')
}

/*function changeColor(newClass, newClassStr, oldClass, oldClassStr, color) {
    newClass.classList.toggle(newClassStr);
    if (newClass.className === newClassStr) {
        oldClass.classList.remove(oldClassStr);
        main.addEventListener('mouseover', function(){
            pickColor(color);
        })
    }
    else {
        main.addEventListener('mouseover', function(){
            pickColor('#333');
        })
    }
}*/
  • 1
    changecolor fires pickcolor repeatedly, which causes new eventlisteners to be created – me_ May 26 '18 at 03:07

2 Answers2

0

You can work out the names but it looks like you're just doing

function go(str1, str2){
    document.getElementById(str1).classList.toggle(str1);
    if (document.getElementById(str1).className === str1) {
        document.getElementById(str2).classList.remove(str2);
        main.addEventListener('mouseover', function(){
            pickColor('#f2f2f2');
        })
    }
    else {
        main.addEventListener('mouseover', function(){
            pickColor('#333');
        })
    }
}

Just call it with go('erase', 'rainbow') or go('rainbow', 'erase')

function eraseGrid(){
    go('erase', 'rainbow');
}

function drawRainbow(){
    go('rainbow', 'erase');
}
Jerinaw
  • 5,260
  • 7
  • 41
  • 54
  • I copied it from his code. At the top of his code snippet you can see he gets erase and rainbow elements by their id. – Jerinaw May 26 '18 at 03:23
0

As for the commented out changeColor function, you should be able to refactor it like this, if you put rainbow and erase in a parent object so you can find them by name.

const objects = {erase, rainbox};

function changeColor(str1, str2) { 
    objects[str1].classList.toggle(str1);
    if (objects[str1].className === str1) {
        objecs[str2].classList.remove(str2);
        main.addEventListener('mouseover', function(){
            pickColor('rgb(' + randColor() + ')');
        })
    }
    else {
        main.addEventListener('mouseover', function(){
            pickColor('#333');
        })
    }
}

Again, as @me_ mentioned in a comment. Adding a eventListener on each click is very likely not what you want. After the fifth click you'll have five eventListeners, and five functions function(){pickColor('rgb(' + randColor() + ')');} that will be be called every time you mouse over "main" (ever expanding for each click...)

(Edit because the comments are so restrictive:)

00Saad: I saw there's a removeEventListener() method, would I implement it by adding main.removeEventListener('mouseover', function(){ pickColor('rgb(' + randColor() + ')'); }) after the addEventListener?

No, store the function away in a variable if you want to be able to easily remove it later.

function(){...} <-- This is the piece of code that creates the new function. So this: main.removeEventListener('mouseover', function(){...}), would create a new anonymous function, and try to remove from main's event-listeners (where it doesn't exists).

Besides, if you use a stored function (const a = function(){..}) and later addEventListener('mouseover', a)) that will kinda solve the problem since you cannot bind the same function to the same event multiple times anyway (it will have no effect).

But then the following addEventListener-calls would make no difference so in your case it would make much more sense to put your mouseover-listener outside the scope of changeColor().

let isRandomColor = true;
main.addEventListener('mouseover', function(){
  pickColor( isRandomColor ? randomColor() : '#333' );
});

function changeColor(){
  if (...){
    isRandomColor = true;
  } else {
    isRandomColor = false;
  }
}
ippi
  • 9,857
  • 2
  • 39
  • 50
  • Why's it bad to have too many event listeners...does it cause memory issues? –  May 26 '18 at 05:47
  • When you add an eventListener you are saying: "Call this function every time I do a 'mouseover` on main". It will keep working until you destroy it (or until you leave the page). So the next time you click the rainbow: "Call this function everytime I do 'mouseover' ". It will create a **new** eventListener, and now, every time when you mouseover main, two functions will trigger. The old eventListener is still there. – ippi May 26 '18 at 06:47
  • This is what you are doing: https://codepen.io/kumorig/pen/QreNMo and also check this question out https://stackoverflow.com/questions/6033821/do-i-need-to-remove-event-listeners-before-removing-elements (good to know about event-listeners) – ippi May 26 '18 at 07:12
  • If it's intentional then it's not bad. But do you *really* want to call `pickColor` 43 times on mouseover? (sorry for the spam!) – ippi May 26 '18 at 07:25
  • I saw there's a removeEventListener() method, would I implement it by adding **main.removeEventListener('mouseover', function(){ pickColor('rgb(' + randColor() + ')'); })** after the addEventListener? –  May 26 '18 at 19:35
  • Edited in an answer to that particular question! – ippi May 27 '18 at 07:47
  • I appreciate everyone's help with this. I didn't bother to remove the event-listener or use a stored function in this particular program, since it works, and I want to move forward (plus it's not something I'm doing professionally), but I'm going to keep this knowledge in mind for future programs. –  May 27 '18 at 17:40