2

What am I doing wrong here (http://jsfiddle.net/dsqBf/2/)?

I'm trying to put the value of the clicked button into the text input. If you click any button, the value of the last button is inserted into the text input.

JavaScript code:

var theButtons = $(".button");
$(theButtons).each(function(index) {
    currentButton = $(this);
    buttonValue = currentButton.val();
    currentButton.click(function() {
        $("#theinput").val(buttonValue);
    });
});

Am I missing a concept I'm not aware of? Thanks!

Rob W
  • 341,306
  • 83
  • 791
  • 678
wkm
  • 1,764
  • 6
  • 24
  • 40

4 Answers4

3

Prefix currentButton by var. Without it, the variable's value will be assigned to a variable in the global scope, because you haven't declared currentButton anywhere else. Consequently, the value of currentButton is changed to the value of the last button (because there's only one variable).

var theButtons = $(".button");
theButtons.each(function(index) {
    var currentButton = $(this);
    var buttonValue = currentButton.val();
    currentButton.click(function() {
        $("#theinput").val(buttonValue);
    });
});

Other notes:

  • thebuttons is already a jQuery object, so you should not wrap it in $ again.
  • $("#theinput") does probably not change over time. So, I recommend to cache this variable.
  • The value of the current button, on the other hand, may change. I suggest to use this.value inside the click handler.
  • Instead of looping using each, you can also bind a click handler on the selector.

Recommended code (demo: http://jsfiddle.net/dsqBf/11/)

var $theButtons = $(".button");
var $theinput = $("#theinput");
$theButtons.click(function() {
    $theinput.val(this.value);
});

Prefixed jQuery-variables with $, because it's the convention to do so. Because of $, you (and others) know that the variable is a jQuery object, which saves expensive debugging time.

Community
  • 1
  • 1
Rob W
  • 341,306
  • 83
  • 791
  • 678
  • Thanks very much. What do you mean to "cache this variable"? – wkm Feb 02 '12 at 16:37
  • 1
    @sloopjohnB As seen at the bottom of the answer, store the jQuery object in a variable outside the loop.click handler. – Rob W Feb 02 '12 at 16:38
3

You are using the .each() instead of the basic .click(). See my update.

http://jsfiddle.net/dsqBf/3/

var theButtons = $(".button");

$(theButtons).click(function() {
    $("#theinput").val($(this).val());
});
Jack
  • 8,851
  • 3
  • 21
  • 26
3

You are experiencing something similar to the closure-in-a-loop problem, because your variables are global.
At the time the event handler is executed, it will access buttonValue which contains the value from the last iteration of the each loop.

There are two ways to solve this: You can either make the variables local, by prepending var, or rewrite the code to:

$(".button").click(function() {
    $("#theinput").val($(this).val());
});
Community
  • 1
  • 1
Felix Kling
  • 795,719
  • 175
  • 1,089
  • 1,143
2

You should do

var theButtons = $(".button");

theButtons.click(function(index) {
    var currentButton = $(this);
    var buttonValue = currentButton.val();

  $("#theinput").val(buttonValue);

});

fiddle here http://jsfiddle.net/dsqBf/6/

Nicola Peluchetti
  • 76,206
  • 31
  • 145
  • 192