Better way to do this?

Hi All,

I feel like there’s a better way to do this.

let imgOne, imgTwo, imgThree, imgFour, imgFive, imgSix;

imgOne      = "<img src='images/facebook.png' class='center'>";
imgTwo      = "<img src='images/twitter.png' class='center'>";
imgThree    = "<img src='images/snapchat.png' class='center'>";
imgFour     = "<img src='images/linkedin.png' class='center'>";
imgFive     = "<img src='images/youtube.png' class='center'>";
imgSix      = "<img src='images/instagram.png' class='center'>";

tdOne.addEventListener('click', () => {
    tdOne.innerHTML = imgOne;
    tdOne.style.background = "white";
});

tdTwo.addEventListener('click', () => {
    tdTwo.innerHTML = imgTwo;
    tdTwo.style.background = "white";
});

tdThree.addEventListener('click', () => {
    tdThree.innerHTML = imgThree;
    tdThree.style.background = "white";
});

tdFour.addEventListener('click', () => {
    tdFour.innerHTML = imgFour;
    tdFour.style.background = "white";
});

tdFive.addEventListener('click', () => {
    tdFive.innerHTML = imgFive;
    tdFive.style.background = "white";
});

tdSix.addEventListener('click', () => {
    tdSix.innerHTML = imgSix;
    tdSix.style.background = "white";
});

I would say that you’re correct, but it’d be more interesting (and educational) for you to share why you feel there is a better way.

I kept thinking “don’t repeat yourself” when adding the event handlers. There must be a way to do this in 1 function and check with conditional/switch statements (maybe?) to see which #id with corresponding block of code should be executed.

Events “bubble up” from the bottom to the top level. So, for example, if you were to [say], put an event listener on the <body>, it would “see” all the events underneath it. Google “event delegation” for Javascript.

2 Likes

This is exactly what i’m looking for. You are my new best friend <3<3<3 !

1 Like

Though I agree with using event delegation., keep in mind there are drawbacks. ie. say if you were to click a child tag instead,. it would not fire off. You’ll need to use .closest() as well.

Do you mean sibling? Because an event on a child will always be picked up by a parent event handler unless explicitly prevented (eg via stopPropagation)

Perhaps I misspoke.

It will fire off but you’re not going to match the proper target.

Example:

<div class="grandparent">
     <div class="parent">
         <div class="title"></title>
     </div>
     <div class="parent">
         <div class="title"></title>
     </div>
</div>

If you did event delegation on the parent divs,. then when a user clicks on the title or any child tag inside of parent., the event.target would be title and not parent. Thus not matching your cases.

So my suggestion was to use .closest

event.target.closest(".parent");

Thus making it slightly longer to find… and you’d have to be sure that another parent element did not have the same class name.

i haven’t checked to see if this actually works but check it out

let names = {
  td_1: "facebook",
  td_2: "twitter",
  td_3: "snapchat",
  td_4: "linkedin",
  td_5: "youtube",
  td_6: "instagram"
};
for(let n in names ){
  // every loop makes a new listener
  createListener(n , names[n])
}
// makes new listeners
function createListener(id, str) {
  // sets image and target
  let image= `<img src='images/${str}.png' class='center'>`;
  let target = document.getElementById(`${id}`)
  // sets the listener
  target.addEventListener("click", () => {
    target.innerHTML = image;
    target.style.background = "white";
  });
}
1 Like

event.target will always give you the correct target, it doesn’t care what the class is, you would need even.target.classList.contains('title') to get the conflict

1 Like

That method is inefficient.

You’re telling me you’re going to do a .contains() for every child element? Which means anytime you add a new child element you’re going to have to open your JS file and add it.

Let me give you a real world example:

<li class="property tile">
   <a href="#" data-selected_property="e41560192c025d6d4818d549272d70ab">
      <div class="tile_info"><span class="more_info"></span></div>
      <div class="flipper">
         <div class="front">
            <div class="img_container">
               <img src="https://twalow.s3-us-west-2.amazonaws.com/2y10MzPpVBHfcfFGShxPY37XIOPluQjXMl0CZcGWXMd6kgnMhvzE52ndi_m" alt="listing_image">
               <div class="listing_name">E Southern Avenue</div>
            </div>
            <div class="listing_content">
               <div class="row_container">
                  <div class="price">$500<span class="per_period">/month</span></div>
                  <div class="shared_status"></div>
                  <div class="reviews">
                     <div class="no_stars_container"><img src="/images/no-star.png" alt="no_star"><img src="/images/no-star.png" alt="no_star"><img src="/images/no-star.png" alt="no_star"><img src="/images/no-star.png" alt="no_star"><img src="/images/no-star.png" alt="no_star"></div>
                  </div>
               </div>
               <div class="mileage">0 mile away</div>
               <div class="location">Tempe, AZ 85281</div>
            </div>
            <div class="background_status"><span>BACKGROUND STATUS</span> <span class="italics">Coming soon.</span></div>
         </div>
         <input type="hidden" name="distance" value="0 mile away"><input type="hidden" name="rating" value="0">
         <div class="back">
            <div class="content">
               <ul>
                  <li>E Southern Avenue</li>
                  <li>
                     <div class="row_container">
                        <div class="icon_label_container">
                           <div class="img_container"><img src="/images/moneybag.png" alt="small_icon"></div>
                           <div class="label">Deposit Required</div>
                        </div>
                        <div class="label_value">$500</div>
                     </div>
                  </li>
                  <li>
                     <div class="row_container">
                        <div class="icon_label_container">
                           <div class="img_container"><img src="/images/deposit_refundable.png" alt="small_icon"></div>
                           <div class="label">Amount Refundable</div>
                        </div>
                        <div class="label_value">---</div>
                     </div>
                  </li>
                  <li>
                     <div class="row_container">
                        <div class="icon_label_container">
                           <div class="img_container"><img src="/images/sublease.png" alt="small_icon"></div>
                           <div class="label">Sublease</div>
                        </div>
                        <div class="label_value">---</div>
                     </div>
                  </li>
                  <li>
                     <div class="row_container">
                        <div class="icon_label_container">
                           <div class="img_container"><img src="/images/utilities_icon.png" alt="small_icon"></div>
                           <div class="label">Utilities</div>
                        </div>
                        <div class="label_value">$0</div>
                     </div>
                  </li>
               </ul>
            </div>
         </div>
      </div>
   </a>
</li>

Say if you wanted to do event delegation on this li.

Are you telling me you’re going to do .contains() on every element in this li?

No,. I hope not. It’s not practical. And whenever you click on any element inside this li., the event.target will equal to the exact element you click.

This is the point i was making. You either have to use click events on each li OR use event delegation but use event.target.closest(".property_tile") and ensure no other elements except for the property tiles is using the class “.property_title”. Otherwise you would get a false positive.

So an example would be if I had another div somewhere on page that used the class name “.property_tile” and it wasn’t really a tile., but if i clicked a child element of that div it would assume I was clicking a property li tile. So class name does matter.

No, you misunderstand. I’m not suggesting that’s what you do, just that event.target gives you the actual event target, it doesn’t care what class is being used. The classList of a particular element is just one field out of many different fields of the object that represents the DOM element. Sure, if you want to switch on classes, and closest is the best option in your particular context, use closest, but that’s not the same thing

2 Likes

What about this:

tdGrid();

function tdGrid() {
    let grid = document.getElementsByTagName("td");

    for(let i = 0; i < grid.length; i++) {
        let cell = grid[i];

        cell.addEventListener('click', () => {     
            cell.innerHTML = randomImage();     
            cell.style.background = "white"; 
        });
    }
}

function randomImage() {
    let imgArr = [
        "<img src='images/facebook.png' class='center'>", 
        "<img src='images/facebook.png' class='center'>",
        "<img src='images/twitter.png' class='center'>",
        "<img src='images/twitter.png' class='center'>",
        "<img src='images/snapchat.png' class='center'>",
        "<img src='images/snapchat.png' class='center'>",
        "<img src='images/linkedin.png' class='center'>",
        "<img src='images/linkedin.png' class='center'>",
        "<img src='images/youtube.png' class='center'>",
        "<img src='images/youtube.png' class='center'>",
        "<img src='images/instagram.png' class='center'>",
        "<img src='images/instagram.png' class='center'>"];

    let random  = imgArr[Math.floor(Math.random() * (imgArr.length - 1))];
    return random;
}

Yes, I know it gives you the actual event target. Everyone knows that.

I think you’re misunderstanding my suggestion.

document.querySelector("body").addEventListener("click", function(e){

      // if clicked element is a child of .property_titles
      if (e.target.closest(".property_tiles")){
                // do action
      }

});

When I say the class matters, I’m saying that it matters when you use closest()., since another group of elements with the same class name would give false positives. So you would be restricted to using unique class names.

This is going round in circles.

  • I’m not
  • you’re suggestion is fine, if you are switching on class names or multiple same properties and you know you want to base that selection on position in the DOM. I am not arguing against it or misunderstanding it.

Then don’t say something different!

Quote me where I say otherwise.

If you did event delegation on the parent divs,. then when a user clicks on the title or any child tag inside of parent., the event.target would be title and not parent . Thus not matching your cases.

As I say, this is going in circles. You seem to have assumed that when I said “…if you were to [say], put an event listener on the <body> , it would “see” all the events underneath it.” I was saying something like “…if you put an event listener on the <body>, then it would see all the events under it and you could select them by checking the class of the event target”. You’ve invented a [common] scenario, then proceeded to correct me as if I was the one who suggested it. There’s nothing wrong with your advice, but it was you who suggested it, not me: it is the correct way to deal with a common scenario, but I have no idea why you thought it was a reply to what I said

How does

If you did event delegation on the parent divs,. then when a user clicks on the title or any child tag inside of parent., the event.target would be title and not parent . Thus not matching your cases.

contradict

Yes, I know it gives you the actual event target. Everyone knows that

The event target is not title. It’s an object that represents a DOM node. title is an entry in an array on a property called classList. I get you think that it’s really obvious that you what meant was really clear, but it didn’t read as that

Ill break it down for you:

I gave this example:

<div class="grandparent">
      <div class="parent">
         <div class="title"></div>
      </div>
      <div class="parent">
         <div class="title"></div>
      </div>
</div>
  1. If you did event delegation on the parent divs,.
  2. then when a user clicks on the title or any child tag inside of parent., the event.target would be title and not parent.
document.querySelector(".grandparent").addEventListener("click", function(e){
     
      console.log(e.target)
      // could be <title></title> or <div class="parent"></div> instead of the intended div ."parent" (all depends where inside div is clicked)

});