How to change the icon of a marker through button click with leaflet?

Question

I have a map with a few markers. Each marker has a infowindow with 3 buttons, like this:

Each button, when clicked changes the icon of the marker. However when I open the infowindow of one marker and don't click on any button,and go to another marker and click in one of the buttons, both markers change icons, instead of changing only the last one cliked. Here is my code:

//Get de todas as ignições presentes na base de dados
$.get("/api/IgnicoesAPI", function (data) {
  //alert(aceite)
  console.log(data);

  $.each(data, function (i, item) {
    //identificação do tipo de marcador que deve aparecer de acordo com o estado da ignição
    var ignicao;

    // MORE CODE

    var id = item.id;

    //colocar um marcador no mapa de acordo com a latitude e longitude fornecidas
    var marker = new L.marker([item.latitude, item.longitude], {
      icon: ignicao,
    })
      .on("click", function onClick(e) {
        //assim que um marcador for clicado é mostrado o popup das ignições
        modal.style.display = "block";

        //indicação do marcador que foi clicado
        clickedmarker = e.target;
        console.log(clickedmarker);

        //vai buscar toda a informação correspondente ao id fornecido
        getData(id);

        //Actividade dos botões presentes no popup das ignições
        $(document).on("click", "#aceite", function () {
          //se o estado for aceite, o botão respetivo estará desativado
          if (item.estado == aceite) {
            document.getElementById("aceite").disabled = true;
            document.getElementById("recusado").disabled = false;
            document.getElementById("concluido").disabled = false;
          }
          //se for clicado passará ao icon correspondente
          else {
            clickedmarker.setIcon(accepted);
            //fecha o modal das avaliação da ignição
            modal.style.display = "none";
            //atualiza a base de dados com o novo estado
            atualizaBD(id, Estado.aceite, item.latitude, item.longitude);
          }
        });

        $(document).on("click", "#concluido", function () {
          //se o estado for concluido, o botão respetivo estará desativado
          if (item.estado == concluido) {
            document.getElementById("concluido").disabled = true;
            document.getElementById("aceite").disabled = false;
            document.getElementById("recusado").disabled = false;
          }
          //se for clicado passará ao icon correspondente
          else {
            clickedmarker.setIcon(conclued);
            //fecha o modal das avaliação da ignição
            modal.style.display = "none";

            //atualiza a base de dados com o novo estado
            atualizaBD(id, Estado.concluido, item.latitude, item.longitude);
          }
        });

        $(document).on("click", "#recusado", function () {
          //se o estado for recusado, o marcador será removido do mapa
          //clickedmarker.removeFrom(map);

          //map.removeLayer(clickedmarker)

          map.removeLayer(marker);

          modal.style.display = "none";
          //atualiza a base de dados com o novo estado
          atualizaBD(id, Estado.recusado, item.latitude, item.longitude);
        });
      })
      .addTo(map);

    //adiciona marador ao mapa
    $("#json map").append(marker);

    if (item.estado == recusado) {
      map.removeLayer(marker);
    }
  }); // fim each
}); //fim do get

How should I solve this problem?

Answer 1

This is a problem about closures and attaching event handlers too eagerly, which ultimately makes your event handlers run more times than you likely wanted.

You're attaching jQuery event handlers on every marker click, so if you have code like...

var marker = new L.marker(/* stuff */).on("click", function onClick(ev) {
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
});

...and you click on a marker, say, 10 times, that has the same effect as attaching the jQuery click marker 10 times:

    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });
    $(document).on("click", "#aceite", function () {
        console.log('running click handler');
    });

So if you then click once on that button, code will run 10 times.

You are getting confused because id exists within the scope of a loop, and the jQuery click handler function is defined within said loop. So if we assume for a moment you've got items with IDs 4 and 5, and you click once on each marker for each item on code like this...

$.each(data, function (i, item) { var id = item.id;

var marker = new L.marker(/* stuff */).on("click", function onClick(ev) {
    $(document).on("click", "#aceite", function () {
        console.log('running click handler with ID', id);
    });
});

...that would be equivalent to attaching two different click event handlers, each of those having a different value for the closure (since they live in different scopes):

    $(document).on("click", "#aceite", function () {
        console.log('running click handler with ID', 4);
    });        
    $(document).on("click", "#aceite", function () {
        console.log('running click handler with ID', 5);
    });

So if you then click once on that button, code will run twice.

Unless you're really sure of what you're doing (i.e. you're keeping track of how many event handlers are attached to an event and are detaching them as needed), avoid attaching event handlers inside loops and inside other event handlers.

So instead of...

data.forEach(function (item, i) {
    var id = item.id;
    L.marker(item.latlng).on('click', function(ev) {
        $("#button").on('click', function() {
            console.log('Doing stuff for item', id);
        });
    });
});

...you should try to keep things supposed to run once (i.e. attaching the jQuery event handler) running once, and hoist any needed state up to a common scope, e.g. ...

// 'id' exists out of the scope of any functions defined inside the loop,
// so it ""exists only once"" to the eyes of those functions
var id;    

data.forEach(function (item, i) {
    L.marker(item.latlng).on('click', function(ev) {
        // Since the marker click handler function is defined within a loop,
        // and the scope of 'item' is that of the loop, it forms a closure, 
        // which means it's ""unique"" to each of the marker click handler
        // functions.
        // By contrast, 'id' is defined outside of that scope, so it's
        // ""common"" to all of the marker click handler functions
        id = item.id;
    });
}); 

// Attach the jQuery event handler **once**, and do not wait
// until clicking on a marker to do so.
$("#button").on('click', function() {
    console.log('Doing stuff for item', id);
});

And please read up on closures. Really.