Javascript function to add onclick event

Question

I am creating a function in JS to create a pagination nav bar. It is suppose to get the total amount of pages and loop through that many times in order to create the proper divs and finally add an onclick function to each created div.

I managed to create the divs with no problem using the for 'i' value to add the ID and innerHTML for each, but the onclick function should also receive the same 'i' value but it won't. I changed the proper function for a simple console.log(i) because the issue is that instead of getting the proper 'i' value the response is 3 in every button. This is the code:

function create_pagination(totalPages, hasNext, hasPrevious) {
  if (document.querySelector('.page-number') != null) {
    document.querySelectorAll('.page-number').forEach((e) => e.remove());
  }

  for (i = 1; i < totalPages + 1; i++) {
    let previousDiv = document.querySelector('#hasPrevious');
    let nextDiv = document.querySelector('#hasNext');
    let parentDiv = document.querySelector('#pagination');

    if (hasPrevious) {
      previousDiv.style.display = 'block';
    }
    if (hasNext && hasPrevious === false) {
      nextDiv.style.display = 'block';
    }

    pageItemDiv = document.createElement('li');
    pageItemDiv.classList.add('page-item');
    pageItemDiv.classList.add('page-link');
    pageItemDiv.classList.add('page-number');

    pageItemNumber = document.createElement('a');
    pageItemNumber.id = `${i}`;
    pageItemNumber.innerHTML = `${i}`;
    pageItemNumber.onclick = () => console.log(i);

    pageItemDiv.appendChild(pageItemNumber);

    parentDiv.insertBefore(pageItemDiv, nextDiv);
  }
}

Thanks in advance for your support!

why did you repeat the sames `previousDiv`, `nextDiv`, `parentDiv` assignation in your loop? — Mister Jojo, Jan 12 '21 at 03:15

Mark · Accepted Answer · 2021-01-12T03:22:32.360

This is a classic issue related to JavaScript 'closure' concept.

Problem

In your loop for (i = 1; i < totalPages + 1; i++), the variable i is bound to the same variable, and after the loop i = 3, thus, when triggering onclick the variable i is bound to value 3, and leads to '3 in every button'.

This article may help you understand the problem as well.

Simple ES6 fix

We only need to use let/const keyword in the loop condition. Check create_pagination_fix1 in the code below.

Reason: docs In for(const i;;){} or for(let i;;){}, i can get a new binding for every iteration of the loop, as each iteration is a new lexical scope. If you use var as for (i = 1 is equal to for (var i = 1, i would get the same binding when you click the button later.

Fix using closure

Please check create_pagination_fix2 in the code below. There have been many posts about the usage of JavaScript closure in the loop, I'd refer you to this post for a nice explanation.

You can run the code snippet below and see the difference between each method. My changes are quite little.

var pageItemDiv = document.getElementById("app");

function create_pagination_problematic(totalPages, hasNext, hasPrevious) {
  if (document.querySelector(".page-number") != null) {
    document.querySelectorAll(".page-number").forEach((e) => e.remove());
  }

  for (var i = 1; i < totalPages + 1; i++) {
    var pageItemNumber = document.createElement("button");
    pageItemNumber.id = `${i}`;
    pageItemNumber.innerHTML = `${i}`;
    pageItemNumber.onclick = () => console.log(i);

    pageItemDiv.appendChild(pageItemNumber);
  }
}

function create_pagination_fix1(totalPages, hasNext, hasPrevious) {
  if (document.querySelector(".page-number") != null) {
    document.querySelectorAll(".page-number").forEach((e) => e.remove());
  }

  // only change is here, use let instead of var
  for (let i = 1; i < totalPages + 1; i++) {
    var pageItemNumber = document.createElement("button");
    pageItemNumber.id = `${i}`;
    pageItemNumber.innerHTML = `${i}`;
    pageItemNumber.onclick = () => console.log(i);

    pageItemDiv.appendChild(pageItemNumber);
  }
}

function getOnclick(i) {
  return function () {
    console.log("Onclick: " + i);
  };
}

function create_pagination_fix2(totalPages, hasNext, hasPrevious) {
  if (document.querySelector(".page-number") != null) {
    document.querySelectorAll(".page-number").forEach((e) => e.remove());
  }

  for (var i = 1; i < totalPages + 1; i++) {
    var pageItemNumber = document.createElement("button");
    pageItemNumber.id = `${i}`;
    pageItemNumber.innerHTML = `${i}`;
    // only changing here: use closure to keep reference to value i
    pageItemNumber.onclick = getOnclick(i);

    pageItemDiv.appendChild(pageItemNumber);
  }
}

create_pagination_problematic(3);
pageItemDiv.appendChild(document.createElement("br"));
pageItemDiv.appendChild(document.createElement("br"));
create_pagination_fix1(3);
pageItemDiv.appendChild(document.createElement("br"));
pageItemDiv.appendChild(document.createElement("br"));
create_pagination_fix2(3);

<!DOCTYPE html>
<html>
  <head>
    <title>Parcel Sandbox</title>
    <meta charset="UTF-8" />
  </head>

  <body>
    <p>
      First row is wrong implementation, when second and third rows are fix.
    </p>
    <div id="app"></div>

    <script src="src/index.js"></script>
  </body>
</html>

Mister Jojo · Answer 2 · 2021-01-12T03:37:46.720

just change

for (i = 1; i < totalPages + 1; i++) {

to

for (let i = 1; i < totalPages + 1; i++) {

your code (cleanned)

create_pagination(4, false, false)

function create_pagination(totalPages, hasNext, hasPrevious)
  {
  document.querySelectorAll('.page-number').forEach(el => el.remove())

  let previousDiv = document.querySelector('#hasPrevious')
    , nextDiv     = document.querySelector('#hasNext')
    , parentDiv   = document.querySelector('#pagination')
    ;
  if (hasPrevious)   previousDiv.style.display = 'block'
  else if (hasNext)  nextDiv.style.display     = 'block'
    ;
  for (let pageN = 1; pageN < totalPages +1; ++pageN) 
    {
    pageItemDiv    = document.createElement('li')
    pageItemNumber = document.createElement('a')

    pageItemDiv.className      = 'page-item page-link page-number'
    pageItemNumber.id          = 
    pageItemNumber.textContent = `${pageN}`
      ;
    pageItemNumber.onclick = () => {console.clear(); console.log(pageN) }

    pageItemDiv.appendChild(pageItemNumber);
    parentDiv.insertBefore(pageItemDiv, nextDiv);
    }
  }

#pagination li { margin: .5em 0;}
#pagination a { display: block; width: 3em; background-color: lightblue;}

<ul id="pagination"></ul>

Javascript function to add onclick event

2 Answers2