CODEWITHSUNDEEP
javascripthtmlcss
Palm
Palm

Reputation: 360

Is this the correct format to write this? It's not working

I'm getting back to learning JavaScript and wondering why this isn't working. No errors are showing in the console.

I made it work with dry coding it but now I'm trying to not use dry and it's not working.

let simonLi = document.querySelector('#simon');
let simonPic = document.querySelector('#simon-pic');
let bruceLi = document.querySelector('#bruce');
let brucePic = document.querySelector('#bruce-pic');
let benLi = document.querySelector('#ben');
let benPic = document.querySelector('#ben-pic');


let pictureChange = pic => {
    if (pic.className === "hide") {
        pic.classList.remove("hide");
    } else {
        pic.classList.add("hide");
    }
};

simonLi.addEventListener('click', pictureChange(simonPic));

bruceLi.addEventListener('click', pictureChange(brucePic));

benLi.addEventListener('click', pictureChange(benPic));

No error messages and it's suppose to hide and show the image whenever the li is click.

Upvotes: 0

Views: 58

Answers (3)

ecc521
ecc521

Reputation: 657

You are attaching the result of the function pictureChange (which happens to be undefined)to the click event, instead of attaching a function.

You can try replacing

let pictureChange = pic => {
    if (pic.classList.contains("hide")) {
        pic.classList.remove("hide");
    } else {
        pic.classList.add("hide");
    }
};

With 

let pictureChange = pic => {
    return function() {
        if (pic.className === "hide") {
            pic.classList.remove("hide");
        } else {
            pic.classList.add("hide");
        }
    }
};

While your at it, consider using pic.classList.toggle() instead.

Upvotes: 2

connexo
connexo

Reputation: 56773

This shortens your code sigificantly:

for (const li of document.querySelectorAll('li')) {
  li.addEventListener('click', (event) => {
    if (event.target.matches('li[id]')) {
       document.getElementById(`${event.target.id}-pic`).classList.toggle('hide');
    }
  })
}

This assumes all your li work the way you've shown. Otherwise, give all those li a common CSS class, e.g. class="li-with-pic" and adjust the querySelector accordingly:

for (const li of document.querySelectorAll('.li-with-pic')) {
  li.addEventListener('click', (event) => {
    if (event.target.matches('.li-with-pic')) {
       document.getElementById(`${event.target.id}-pic`).classList.toggle('hide');
    }
  })
}

Upvotes: 0

muka.gergely
muka.gergely

Reputation: 8329

This might work better:

let simonLi = document.querySelector('#simon');
let simonPic = document.querySelector('#simon-pic');
let bruceLi = document.querySelector('#bruce');
let brucePic = document.querySelector('#bruce-pic');
let benLi = document.querySelector('#ben');
let benPic = document.querySelector('#ben-pic');

function pictureChange(pic) {
  pic.classList.toggle('hide')
}

simonLi.addEventListener('click', function(e) {
  pictureChange(simonPic)
});

bruceLi.addEventListener('click', function(e) {
  pictureChange(brucePic)
});

benLi.addEventListener('click', function(e) {
  pictureChange(benPic)
});
.hide {
  color: red;
}
<div id="simon"><span id="simon-pic">Simon</span></div>
<br />
<div id="bruce"><span id="bruce-pic">Bruce</span></div>
<br />
<div id="ben"><span id="ben-pic">Ben</span></div>

Upvotes: 1

Related Questions