2

The task is to fetch some data from pokemon api and append it to the list. Api request results have links on previous and next pages.

HTML:

<ul class="poke-list"></ul>
<div class="pagination">
   <button class="prev">Prev</button>
   <button class="next">Next</button>
</div>

here's a function that makes an api call (async await is necessary):

var getData = async function (url, pokemonName) {
   var response;

   if(!pokemonName) {
     response = await $.get(url);
   } else {
     response = await $.get(url + pokemonName);
   }
     return response;
}; 

A function to append content and handle click events:

var appendContent = function (data) {
  var list = $('.poke-list');
   list.empty();

   var res = data;
   var pokemons = res.results;
   pokemons.forEach(function (item) {
     list.append($(`<li>${item.name}</li>`));
  $('.prev').on('click', function (event) {
    res = data;
    var url2 = res.previous;
    if (url2 === undefined || url2 === null) {
      alert('Limit reached!');
    } else {
       getData(url2)
        .then(appendContent);
    }
   });

  $('.next').on('click', function (event) {
     res = data;
     var url = res.next;
     if (url === undefined || url ===null) {
        alert('Limit reached!');
     } else {
        getData(url)
          .then(appendContent)
  });

});

I call it on page load (yes it is necessary):

$(function {
  getData()
    .then(appendcontent)

here is a fiddle: https://jsfiddle.net/aikast/4rgcvd7z/

What happens is that every time append function is called it creates new click events (I know that's how it should work), and the code stops working properly (it does not see current ajax call results, so every value is null).

Stopping event propagation did not help.

Is there a way to organise it differently? I can't find a way for click event handlers to see ajax call results outside of appendContent function scope

The question was marked duplicate, but buttons are static, they are not dynamically added!

Aika Sat
  • 167
  • 3
  • 11
  • The main problem is that you keep adding event listeners to the two buttons, which means when you click prev/next the first time, it works fine, but clicking any of them again will run the click handler twice, then thrice, etc. Also, you need `then(appendcontent)` in your page load code. –  Oct 22 '19 at 09:17
  • yes, that is exactly what my problem is, but I need those api call results in click event handlers, so they must be in a function that gets/sees those results, right? – Aika Sat Oct 22 '19 at 09:21
  • Possible duplicate of [Attach event to dynamic elements in javascript](https://stackoverflow.com/questions/34896106/attach-event-to-dynamic-elements-in-javascript) – hindmost Oct 22 '19 at 09:21
  • buttons are not dinamically added! please re-read – Aika Sat Oct 22 '19 at 09:24
  • I have no need in click events for dynamic content! – Aika Sat Oct 22 '19 at 09:28
  • 2
    The solution is to use external variables that are *set* after the API data is received and *used* when a button is clicked. Which means this question is most definitely a duplicate, just not of the ones people thought it was. Here's fixed code: https://jsfiddle.net/khrismuc/myo8qtve/ (note that `var = function() { ...};` is a bad style because that way the function isn't hoisted) –  Oct 22 '19 at 09:34
  • yeah, I would be glad if it was marked duplicate by a correct link you saved the day! – Aika Sat Oct 22 '19 at 09:44
  • so the best way to add functions is through function declaration, not through function expressions right? – Aika Sat Oct 22 '19 at 09:47
  • Declaring a variable external to make it available to more than one function is a basic programming concept. Finding a duplicate is not easy because if you take your own question as example, people are usually asking a very different thing. Anyway, yes, you shouldn't declare functions as `var`s. –  Oct 22 '19 at 09:55
  • This probably comes closest: [What is the scope of variables in JavaScript?](https://stackoverflow.com/questions/500431/what-is-the-scope-of-variables-in-javascript) –  Oct 22 '19 at 09:55

2 Answers2

1

you should try something like below. now only once click event fired.

$(next).unbind( "click" );

so every time when your function call, first it will unbind previous event.

you can check live demo as well.

https://jsfiddle.net/zhna7ksu/


.unbind() is deprecated and you should use the .off() method instead. Simply call .off() right before you call .on()

$(next).off().on("click", .......);
Vishal modi
  • 1,571
  • 2
  • 12
  • 20
  • While this will work, it ignores the actual problem (which is about variable scope, not click handling) –  Oct 22 '19 at 09:56
  • can you explain, what is the problem in above solution? – Vishal modi Oct 22 '19 at 09:57
  • The problem is that it completely ignores the bigger picture. Assigning (and therefore unbinding) a click handler after each API request is not necessary in the first place. If your bed is getting dirty from your shoes, the solution isn't to put plastic bags over your shoes ;) –  Oct 22 '19 at 09:59
  • @ChrisG why don't you take of your shoes. in other words why are you binding event in appendContent (take it out from there). – Ejaz47 Oct 22 '19 at 10:04
  • @Ejaz47 I'm not the OP; I already provided a [solution](https://jsfiddle.net/khrismuc/myo8qtve/) that does exactly that. –  Oct 22 '19 at 10:07
0

store data/res (why double naming the same variable?) somewhere outside appendData (window.pokemonResponse for example) function context and declare previous and next logic outside appendData. Also, use let instead of var, even IE 11 supports let

Simon
  • 1,280
  • 3
  • 10
  • 21