8

Context

I'm trying to implement a feature so that when the user clicks on a checkbox within a table, the attribute value and data-title of the checkbox should be stored in a JS object literal named selected as a new key-value pair array element.

In case the user clicks a second time on the same checkbox, the corresponding array element should be removed.

Issue

The first time a checkbox is clicked, an array is created in object selected as intended.

However, when the same checkbox is clicked a second time, instead of removing the corresponding array, a new one (repeated) is added.

Code

var selected = {items:[]};     
$('#table').on('click', 'input[type="checkbox"]', function() {
    var found = false;
    $.each(selected.items, function(i, val) {
        if (val.key == $(this).attr("value")) {
            selected.items.splice(i ,1);
            found = true;
            return false; //step out of each()
        }
    });

    if (found == false) {
        selected.items.push({key: $(this).attr("value"), value: $(this).attr("data-title")});
    }

    console.log(selected);
});
Sam32
  • 117
  • 1
  • 8
  • Why not make it depend on the `checked` state of the checkbox? I assume your array should represent the boxes that are currently checked. – KWeiss Feb 09 '16 at 15:43
  • 1
    It's likely an issue with `val.key == $(this).attr("value")`. This likely (possibly due to the strangeness of equality in Javascript), never returns true. You should probably use [===](http://stackoverflow.com/questions/359494/does-it-matter-which-equals-operator-vs-i-use-in-javascript-comparisons) aswell. [Debug your javascript](http://stackoverflow.com/questions/988363/how-can-i-debug-my-javascript-code) and check this – Liam Feb 09 '16 at 15:44
  • Nicely written question BTW, well done. If only every low rep user was this considerate – Liam Feb 09 '16 at 15:45
  • 3
    What is `this` equal to inside the `$.each` callback? – Davin Tryon Feb 09 '16 at 15:46
  • Just rebuild the array. Other option is to check the checked state. If checked, do what you are doing. If unchecked loop and find the match and remove that index. – epascarello Feb 09 '16 at 15:47

3 Answers3

6

You have the wrong context of this inside each. it is no longer the element in the click handler

Try

$('#table').on('click', 'input[type="checkbox"]', function() {
    var found = false;
    var value = $(this).val();// store value from `this` before entering `each`
    $.each(selected.items, function(i, val) {
        if (val.key == value) {
            selected.items.splice(i ,1);
            found = true;
            return false; //step out of each()
        }
    });
    ....
charlietfl
  • 170,828
  • 13
  • 121
  • 150
  • That's exactly the root cause of the problem. – Sam32 Feb 09 '16 at 16:02
  • @Sam32 should definietly use `===` as liam pointed out. I didn't want to change your code a lot...just point out the *"why"* which is more important sometimes than learning all new ways to do it and not getting to root of original problem – charlietfl Feb 09 '16 at 16:05
3

First, I would recommend to use a key-value-pair object, as it is way easier for lookups.

var selected = { items : {} };

this way you would access your selected items using

selected.items[my.key]

maybe somethink like this...

var selected = {items:{}};     
$('#table').on('change', 'input[type="checkbox"]', function() {
    var checked = $(this).is(":checked"),
        title = $(this).data("data-title"),
        value = $(this).val();

    if (checked && !selected.items[value])
        selected.items[value] = title;
    else if (!checked && !!selected.items[value])
        delete selected.items[value];
});
Luke
  • 8,235
  • 3
  • 22
  • 36
0

how about this one:

var selected = { items:[] };

$('#table').on('click', 'input[type="checkbox"]', function() {
    selected.items = $('#table input[type="checkbox"]:checked').map(function(){
        return {
            key: $(this).attr('value'),
            value: $(this).attr('data-title')
        }
    }).toArray()
});
Thomas
  • 3,513
  • 1
  • 13
  • 10