Connecting JS functions with HTML elements

JS works in HTML document fine using the Script tag, but trying to place JS in external file it doesn’t work. Yes, I know about the src .js, just like CSS, but do I need to create function() and link to HTML onclick to get the external function to work?

Update, got the Function to work but the Accordion only works on 2 clicks and not 1? Any ideas? Thanks.

  var acc = document.getElementsByClassName("accordion");
  var i;
  
  function myAcc(){
  for (i = 0; i < acc.length; i++) {
    acc[i].addEventListener("click", function() {
      this.classList.toggle("active");
      var panel = this.nextElementSibling;
      if (panel.style.maxHeight) {
        panel.style.maxHeight = null;
      } else {
        panel.style.maxHeight = panel.scrollHeight + "px";
      } 
    });
  }}

It seems that your first click is used to run the myAcc() function. That function adds the event listener.
Then your second click also runs the myAcc() function and also the event listener that was added by the addEventListener line from the previous time that the myAcc() was run.

It seems to me, without knowing more about now the page works, that you need to invoke that myAcc() function from the end of the document.

If you load the script from the end of the body, which is the normal place, then you can invok that myAcc() function immediately after defining it.

Welcome to the forums, @bellerjeff

[off-topic]
When you post code in the forum, you need to format it. To do so you can either select all the code and click the </> button, or type 3 backticks ``` on a separate line both before and after the block of code.

I have done it for you this time.
[/off-topic]

1 Like

Thank you and I will.

1 Like

Thank you Paul. I’ve tried moving the .SRC in my HTML to the bottom but hasn’t fixed. I’m still learning and don’t understand. Starting to understand that two different states (invoked and called) but I don’t know how to fix it. The JS code works inside the HTML doc but not as an external file, I’ve been trying to read/understand your comments but still can’t get it to work.

Ha. Well, I got accordian to open on 1 click by adding a ‘’’ window.onload’‘’ in my external file, so that’s good. But there is still a double-click? Any ideas on how to make it a single?

I have here a simplified version of the problem that you’re facing, where the first click doesn’t seem to do anything but results in adding event handlers, and subsequent clicks trigger those event handlers.

<p id="bunny">Bunny is <span class="feedstate">unfed</span>.</p>
<p><button class="feed" data-id="bunny" onclick="myFeeds()">Feed bunny</button></p>
<p id="mouse">Mouse is <span class="feedstate">unfed</span>.</p>
<p><button class="feed" data-id="mouse" onclick="myFeeds()">Feed mouse</button></p>
<p id="hamster">Hamster is <span class="feedstate">unfed</span>.</p>
<p><button class="feed" data-id="hamster" onclick="myFeeds()">Feed hamster</button></p>
var feeds = document.getElementsByClassName("feed");
var i;
function myFeeds() {
  for (i = 0; i < feeds.length; i++) {
    feeds[i].addEventListener("click", function() {
      var creature = document.querySelector("#" + this.dataset.id);
      creature.classList.add("active");
      creature.querySelector(".feedstate").innerHTML = "fed"
    });
  }
}
p.active {
  background-color: lightgreen;
}

There are a number of problems in that code, but first of all are the inline attribute click events. Those really need to be moved in to the scripting code instead.

Move events out of HTML and in to JavaScript

Remove inline attribute click events from the HTML code:

<!--<p><button class="feed" data-id="bunny" onclick="myFeeds()">Feed bunny</button></p>-->
<p><button class="feed" data-id="bunny">Feed bunny</button></p>
...
<!--<p><button class="feed" data-id="mouse" onclick="myFeeds()">Feed mouse</button></p>-->
<p><button class="feed" data-id="mouse">Feed mouse</button></p>
...
<!--<p><button class="feed" data-id="hamster" onclick="myFeeds()">Feed hamster</button></p>-->
<p><button class="feed" data-id="hamster">Feed hamster</button></p>

and add those events into the JavaScript code.

for (i = 0; i < feeds.length; i++) {
  feeds[i].addEventListener("click", myFeeds);
}

Here is the updated code. https://jsfiddle.net/kvo9hgj0/1/

Examine the Cause of the Problem

The two-click problem is still occurring, but now it’s easier to see in the scripting code what is causing that problem.

var feeds = document.getElementsByClassName("feed");
var i;

function myFeeds() {
  for (i = 0; i < feeds.length; i++) {
    feeds[i].addEventListener("click", function() {
      var creature = document.querySelector("#" + this.dataset.id);
      creature.classList.add("active");
      creature.querySelector(".feedstate").innerHTML = "fed"
    });
  }
}

for (i = 0; i < feeds.length; i++) {
  feeds[i].addEventListener("click", myFeeds);
}

There are two sets sets of event listeners being added, when only one set of them is needed.

Remove the Other Event Listener

By removing the event listener in the myFeeds() function, the code inside of that event listener won’t be delayed until you next click on something, but will be free to run and do its work straight away.

function myFeeds() {
  // for (i = 0; i < feeds.length; i++) {
  //   feeds[i].addEventListener("click", function() {
      var creature = document.querySelector("#" + this.dataset.id);
      creature.classList.add("active");
      creature.querySelector(".feedstate").innerHTML = "fed"
  //   });
  // }
}

We are then left with just the code that feeds the creature:

function myFeeds() {
  var creature = document.querySelector("#" + this.dataset.id);
  creature.classList.add("active");
  creature.querySelector(".feedstate").innerHTML = "fed"
}

It is now just one click on each button to feed each of them. https://jsfiddle.net/kvo9hgj0/2/

image

A tidy up of further issues in that code involves bringing together the variables with the loop that uses them:

var feeds = document.getElementsByClassName("feed");
var i;
for (i = 0; i < feeds.length; i++) {
  feeds[i].addEventListener("click", myFeeds);
}

then we can use querySelectorAll to get the feeds:

var feeds = document.querySelectorAll(".feed");
var i;
for (i = 0; i < feeds.length; i++) {
  feeds[i].addEventListener("click", myFeeds);
}

Then because the element list from querySelectorAll supports forEach, we can completely replace the for loop with a forEach loop instead:

var feeds = document.querySelectorAll(".feed");
var i;
feeds.forEach(function (button, i) {
  feeds[i].addEventListener("click", myFeeds);
});

And then we can remove the need for the i variable completely, as the button parameter can be used instead.

var feeds = document.querySelectorAll(".feed");
feeds.forEach(function (button) {
  button.addEventListener("click", myFeeds);
});

And lastly, we should rename the myFeeds() function to something a but more meaningful, such as feedPet() instead.

// function myFeeds() {
function feedPet() {
  ...
}

var feeds = document.querySelectorAll(".feed");
feeds.forEach(function (button) {
  // button.addEventListener("click", myFeeds);
  button.addEventListener("click", feedPet);
});

That leaves us with the updated code at https://jsfiddle.net/kvo9hgj0/5/

Copy and paste error I think :slight_smile: I believe it should be:

var feeds = document.querySelectorAll(".feed");

2 Likes

Eek! Do not look behind the curtain. There’s nothing swept under the carpet either!

3 Likes