1

Begginner here: Can someone please help simplify this script? I want to change all the elements on the keeper div when clicking on the div of a player (guilherme ou gonçalo). I found a way to do this but I keep on repeating myself. Is there a way to simplify?

var goncalo = document.getElementById("goncalo");
var grj1 = document.getElementById("j1");
var j1img = document.getElementById("j1img")
var guilherme = document.getElementById("guilherme");

goncalo.addEventListener("click", function replaceContent() {
  grj1.innerHTML = "<h6>Gonçalo</h6>";
});
goncalo.addEventListener("click", function changeSrcAttribute() {
  j1img.setAttribute("src", "Jogadores/gonçalo.jpg");
});

guilherme.addEventListener("click", function replaceContent() {
  grj1.innerHTML = "<h6>Guilherme</h6>";
});
guilherme.addEventListener("click", function changeSrcAttribute() {
  j1img.setAttribute("src", "Jogadores/Guilherme.jpg");
});
<div id="Keeper">
  <img id="j1img" src="Jogador.png" alt="Keeper">
  <h6 id="j1">GK</h6>
</div>

<div class="gk">
  <div id="guilherme">
    <img id="guilhermeimg" src="Jogadores/Guilherme.jpg" alt="Guilherme">
    <h6 id="gui">Guilherme</h6>
  </div>
  <div id="goncalo">
    <img id="goncaloimg" src="Jogadores/gonçalo.jpg" alt="Gonçalo">
    <h6 id="gon">Gonçalo</h6>
  </div>
Barmar
  • 741,623
  • 53
  • 500
  • 612
  • 2
    Use [event delegation](//developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events#Event_delegation) instead of assigning multiple event listeners — it’s more maintainable, and applies to dynamically added elements. E.g., use an [event argument](//developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#The_event_listener_callback)’s [`target`](//developer.mozilla.org/en-US/docs/Web/API/Event/target). See [the tag info](/tags/event-delegation/info) and [What is DOM Event delegation?](/q/1687296/4642212). – Sebastian Simon Jun 09 '21 at 03:05
  • 1
    What was wrong with the solutions for this same UI we gave you a few days ago? I gave you a generic class based solution that doesn't require you to write id specific code the way you are doing again. When you have numerous repeating components in a page you can't scale using only ids for selectors – charlietfl Jun 09 '21 at 03:14
  • It worked at first but i couldnt make it to change the style too. This way i've made it. But it was a huge help anyway – Francisco Serra Jun 09 '21 at 12:32
  • @FranciscoSerra you need to: 1. go to this link: [**XY Problem**](https://xyproblem.info/). 2. go to this link: [***How do I ask a good question?***](https://stackoverflow.com/help/how-to-ask). If you believe that those two links do not apply, please repost your question and all future questions to one of these two links: [**Stackoverflow ES**](https://es.stackoverflow.com/) or [**Stackoverflow PT**](https://pt.stackoverflow.com/). – zer00ne Jun 09 '21 at 16:38
  • **RE:** styles. Like the `#id` attribute, `[style]` attribute is another anti-pattern. Please go to this link: [**Styles and classes**](https://javascript.info/styles-and-classes). BTW, don't use `#id`, `.setAttribute()` and `.getElementById()` to do everything. There is `.class`, `.classList()`, `.querySelector()` (for multi elements `.querySelectorAll()`). If you don't want to write the same pieces of code for each slightly different element, you need to go from specific selection to general selection -- from `#id` to `.class`. – zer00ne Jun 09 '21 at 16:58

3 Answers3

2

Before reading this answer, please refer to Sebastian Simon's comment above. Also, if you insist on using #ids don't bother to consider this answer. The only time the use of #ids is advantageous is in specific cases in the use of forms and form controls. The use (abuse?) of #id in OP is ludicrous, please refer to charlietfl's comment above.

Avoid hard coding unique content like text or urls. Define unique content in a data structure (ex. array and/or object) outside of your functions so you can change them without touching the rest of the code. Of course event handlers (functions designed to run when an event occurs) only pass on the Event Object (ex. clickHandler(event)), but there are workarounds such as wrapping the event handler definition in another function -- this function is known as a closure.

In the following example there is a closure init(string, array1, array2) which has three required parameters:

  1. Generic: string Descriptive: listener | a selector string of a single element that contains all of the <img>s
    /*Examples*/ "main", "body", ".images", "#try_Not_To_Use_Ids_They_Criiple_U"
    
  2. Generic: array1 Descriptive: srcArray | an array of urls that point to images
  3. Generic: array2 Descriptive: txtArray | an array of texts for captions

These parameters are defined outside of init() so you won't have to change anything but these 3 parameters if the need arises. There are a couple of requirements in the HTML layout as well:

  1. The number of <img>s, and the .length of both srcArray and txtArray must be equal. event.target property will determine exactly which <img> was clicked and will be referenced as clicked.
  2. Each <img> must be followed by an element that can contain text. These elements will generically referenced as clicked.nextElementSibling
    /*Example*/ <div></div>, <figcaption></figcaption>, <h6></h6>,....
    
  3. All <img> and .nextElementSiblings must be within the listener (referenced element in parameter 1).

This example is very agnostic. #ids and .classes are never needed and the amount of <img>s is virtually unlimited (initially and dynamically).

Details are commented in example

const srcArray = ['https://static.wikia.nocookie.net/disney/images/6/65/Profile_-_General_Grievous.png/revision/latest?cb=20190313134830', 'https://pbs.twimg.com/profile_images/3103894633/e0d179fc5739a20308331b432e4f3a51.jpeg', 'https://pbs.twimg.com/profile_images/639566884807901185/LFKhqvgo_400x400.jpg'];

const txtArray = ['General Grievous', 'Darth Vader', 'Kylo Ren'];

function init(listener, srcA, txtA) {
  // Reference listener
  const parentNode = document.querySelector(listener);

  /*
  Collect all <img>s within listener into a NodeList   
  (referenced as imgList)
  for Each <img> in imgList add [data-idx] attribute and
  set the value to it's index number
  Result:  <img data-idx="0">, <img data-idx="1">,...
  */
  /* Note: if in the future you are to dynamically add
    <img>s and caption elements, move the next two lines
    of code into clickHandler()
  */
  const imgList = parentNode.querySelectorAll('img');
  imgList.forEach((img, index) => img.dataset.idx = index);

  // Event handlers pass the Event Object by default 
  function clickHandler(event) {
    // Reference of the specific element user has clicked
    const clicked = event.target;
    // if user clicked an <img>...
    if (clicked.matches('img')) {
      /*
      - get it's [data-idx] value and convert it into a
        real number...
      - get the url from srcArray at that [data-idx]
        number and set <img> [src] with it...
      - get the text string from txtArray at that
        [data-idx] number and inset it into the element
        that follows <img> (<figcaption> in this example)
      Result: 
        <img src="https://pbs.twimg.com/profile_images/639566884807901185/LFKhqvgo_400x400.jpg" data-idx="2">
        <figcaption>Kylo Ren</figcaption>
      */
      let idx = parseInt(clicked.dataset.idx);
      clicked.src = srcA[idx];
      clicked.nextElementSibling.textContent = txtA[idx];
    }
  };
  // Register parentNode as the click listener
  parentNode.onclick = clickHandler;
};

/*
Run init()
1. Passing selector of "main" as the listener
2. Passing the predefined string arrays
3. All <img>s within parentNode/listener will have 
   [data-idx] = index position number
4. listener will be registered to the 'click' event
*/
init('main', srcArray, txtArray);
body {
  font: 2ch/1.25 Consolas
}

main {
  display: flex;
  justify-content: space-between;
  width: 90vw;
  height: 90vh;
  padding: 10px;
}

figure {
  margin: 5px;
}

img {
  display: block;
  width: 150px;
  height: 150px;
  cursor: pointer;
}

figcaption {
  text-align: center;
}
<main>
  <figure>
    <img src='https://via.placeholder.com/150'>
    <figcaption></figcaption>
  </figure>
  <figure>
    <img src='https://via.placeholder.com/150'>
    <figcaption></figcaption>
  </figure>
  <figure>
    <img src='https://via.placeholder.com/150'>
    <figcaption></figcaption>
  </figure>
</main>
zer00ne
  • 41,936
  • 6
  • 41
  • 68
1

Define a named function that you can call in both event listeners, and give it parameters for the differences.

And there's no need to have two event listeners for the same event, just put both actions in one listener.

var goncalo = document.getElementById("goncalo");
var grj1 = document.getElementById("j1");
var j1img = document.getElementById("j1img")
var guilherme = document.getElementById("guilherme");

function replaceContentAndSrc(content, src) {
  grj1.innerHTML = `<h6>${content}</h6>`;
  j1img.setAttribute("src", src);
}

goncalo.addEventListener("click", function() {
  replaceContentAndSrc("Gonçalo", "Jogadores/gonçalo.jpg");
});

guilherme.addEventListener("click", function() {
  replaceContentAndSrc("Guilherme", "Jogadores/Guilherme.jpg");
});
<div id="Keeper">
  <img id="j1img" src="Jogador.png" alt="Keeper">
  <h6 id="j1">GK</h6>
</div>

<div class="gk">
  <div id="guilherme">
    <img id="guilhermeimg" src="Jogadores/Guilherme.jpg" alt="Guilherme">
    <h6 id="gui">Guilherme</h6>
  </div>
  <div id="goncalo">
    <img id="goncaloimg" src="Jogadores/gonçalo.jpg" alt="Gonçalo">
    <h6 id="gon">Gonçalo</h6>
  </div>
</div>
Barmar
  • 741,623
  • 53
  • 500
  • 612
0

you can do that,
with event delegation

const 
  KeeperElm_Img = document.querySelector('#Keeper > img')
, KeeperElm_H6  = document.querySelector('#Keeper > h6')
, div_gk        = document.querySelector('div.gk')
  ;
div_gk.onclick = ({target}) =>
  {
  let clicked = target.matches('img, h6') ? (target.closest('div') ?? null) : target

  // if (!clicked.matches('#guilherme, #goncalo')) return
  if (!clicked.matches('div.gk > div')) return

  KeeperElm_Img.src        = clicked.querySelector('img').src
  KeeperElm_H6.textContent = clicked.querySelector('h6').textContent
  }
div.gk > div {
  background : lightblue; 
  }
h6 {
  margin-top: 0.2em;
}
<div id="Keeper">
  <img src="https://loremicon.com/rect/64/64/318424456936/jpg" alt="Keeper">
  <h6 >GK</h6>
</div>

<div class="gk">
  <div id="guilherme">
    <img src="https://loremicon.com/ngon/64/64/769636094518/jpg" alt="Guilherme">
    <h6 >Guilherme</h6>
  </div>
  <div id="goncalo">
    <img src="https://loremicon.com/ngon/64/64/13009077523/jpg" alt="Gonçalo">
    <h6>Gonçalo</h6>
  </div>
  <div>
    <img src="https://loremicon.com/poly/64/64/943977855439/jpg" alt="poly">
    <h6>poly</h6>
  </div>
</div>
Mister Jojo
  • 20,093
  • 6
  • 21
  • 40