1

I have three different boxes which all have a button inside.

I want to register an onclick event for every button with a specific data attribute.

I've started a jsfiddle, any suggestions?

http://jsfiddle.net/mwils/w6gofb30/

var buttons = document.querySelectorAll("button[data-donation]");

for (i = 0; i < buttons.length; i++) {
    document.querySelectorAll("button[data-donation]")[i].onclick = console.log(buttons[i]);
}

function letsGo(init) {

    var input = document.body.querySelector("input[name='amount']"),
        amount = parseInt(input.value, 10) * 100,
        url = "https://donate.shelter.org.uk/b?cid=152&freeamount=1&amount=";

    if (input.value === '' || parseInt(input.value, 10) >= 0) {
        window.location.href = url + init * 100;
    } else {
        window.location.href = url + amount;
    }
}
<div class="shopping-item">
    <p><span>£33</span> could help a family find and keep a home</p>
    <input type="number" name="amount" placeholder="£33" class="shopping-1-input">
    <button data-donation="33" class="donate-button shopping-1-button">donate now</button>
</div>
    
<div class="shopping-item">
    <p><span>£50</span> could help a family find and keep a home</p>
    <input type="number" name="amount" placeholder="£50" class="shopping-2-input">
    <button data-donation="50" class="donate-button shopping-2-button">donate now</button>
</div>
    
<div class="shopping-item">
    <p><span>£188</span> could help a family find and keep a home</p>
    <input type="number" name="amount" placeholder="£188" class="shopping-3-input">
    <button data-donation="188" class="donate-button shopping-3-button">donate now</button>
</div>
epascarello
  • 204,599
  • 20
  • 195
  • 236
Michael Wilson
  • 1,548
  • 3
  • 21
  • 44

5 Answers5

3

While there are really many ways to do that i, most of the time, prefer a simple click delegation. I set that on elements that contain the buttons(could be the window or document.body itself).

To make it simple i show you that with a container first.

<div id="container">
 <button>1</button>
 <button>2</button>
 <button>3</button>
 <button>4</button>
 <button>5</button>
</div>

here is the js

document.getElementById('container').onclick=function(e){
 if(e.target.nodeName=='BUTTON'){
  alert(e.target.textContent);
 }
}

What does this???

if i click on the container it checks if the target element is a button. if yes it alerts the textContent of the button. simple right? Doing so you avoid alot of extra variables.


In your case

document.onclick=function(e){
 if(e.target.dataset['donation']){
  alert(e.target.dataset['donation']);
 }
}

shorter

document.onclick=function(e){
 e=e.target.dataset; // reuse the variable e
 !e['donation']||alert(e['donation']);
}

using

<button data-donation="33">donate now</button>

DEMO's

http://jsfiddle.net/j8xgqfmk/

Extra

button text followed by $ symbol done with css

http://jsfiddle.net/j8xgqfmk/1/

preceded by a £

http://jsfiddle.net/j8xgqfmk/2/

but returning the desidered value

Why?

  1. No loops!!!
  2. Only one event handler!!!!
  3. Less variables!!!
  4. Short code !!!
  5. Faster then multiple complex eventhandlers.

other considerations:

var buttons = document.querySelectorAll("button[data-donation]");

for (i = 0; i < buttons.length; i++) {
 document.querySelectorAll("button[data-donation]")[i].onclick=console.log(buttons[i]);
}

Should be written

var buttons=document.querySelectorAll('button[data-donation]'),
    btnlength=buttons.length; //cache variables

for (var i = 0; i < btnlength; i++) { // var i
 buttons[i].onclick=function(){ //buttons is already defined
  console.log(this);
 }
 // and you need to pass a function to the onclick event
}

or even better

 function handler(e){
  console.log(this,e);
 }

 var btns=document.querySelectorAll('button[data-donation]'),
     l=btns.length;

 while(l--)btns[l].addEventListener('click',handler,false);

or the "Haters gonna hate" version

http://jsfiddle.net/Lbk1ueme/

var B=document.querySelectorAll('button[data-donation]'),
    L=B.length,
    I=0,
    H=function(){console.log(this)};

for(;I<L;B[I++].onclick=H);

https://stackoverflow.com/a/21353032/2450730

if you have difficulties to understand that i can help you to write it based on your requests. For any other question just ask.

keep stuff simple

Community
  • 1
  • 1
cocco
  • 16,442
  • 7
  • 62
  • 77
  • `buttons[i].onclick=console.log(buttons[i]);` it will log in `console` when this `for` is working but won't be `onclick` function. you must make `buttons[i].onclick=function(){console.log(buttons[i])};` @cocco – Guja1501 May 19 '15 at 12:55
1

Loop through your element array (as you already are doing) and use addEventListener to bind the click event to each...

var buttons = document.querySelectorAll("button[data-donation]");

for (i = 0; i < buttons.length; i++) {
    buttons[i].addEventListener('click', function(event) {
        alert(this.getAttribute('data-donation')); // alert the value of data-donation attribute
    });
}
<div class="shopping-item">
    <p><span>£33</span> could help a family find and keep a home</p>
    <input type="number" name="amount" placeholder="£33" class="shopping-1-input">
    <button data-donation="33" class="donate-button shopping-1-button">donate now</button>
</div>
    
<div class="shopping-item">
    <p><span>£50</span> could help a family find and keep a home</p>
    <input type="number" name="amount" placeholder="£50" class="shopping-2-input">
    <button data-donation="50" class="donate-button shopping-2-button">donate now</button>
</div>
    
<div class="shopping-item">
    <p><span>£188</span> could help a family find and keep a home</p>
    <input type="number" name="amount" placeholder="£188" class="shopping-3-input">
    <button data-donation="188" class="donate-button shopping-3-button">donate now</button>
Turnip
  • 35,836
  • 15
  • 89
  • 111
1

The problem is you are not attaching events correctly to the element. You are assigning console.log and not a function to it. Seconds querySelectorAll is expensive. You should not keep looking things up in a loop. Create a variable and store the live html node collection it returns into it.

var btns = document.querySelectorAll("button[data-donation]");
for (var i=0; i<btns.length; i++) {
    btns[i].onclick = function() {
        console.log(this);
        console.log(this.getAttribute("data-donation"));
    }
}

function letsGo(init) {

    var input = document.body.querySelector("input[name='amount']"),
        amount = parseInt(input.value, 10) * 100,
        url = "https://donate.shelter.org.uk/b?cid=152&freeamount=1&amount=";

    if (input.value === '' || parseInt(input.value, 10) >= 0) {
        window.location.href = url + init * 100;
    } else {
        window.location.href = url + amount;
    }
}
<div class="shopping-item">
    <p><span>£33</span> could help a family find and keep a home</p>
    <input type="number" name="amount" placeholder="£33" class="shopping-1-input">
    <button data-donation="33" class="donate-button shopping-1-button">donate now</button>
</div>
    
<div class="shopping-item">
    <p><span>£50</span> could help a family find and keep a home</p>
    <input type="number" name="amount" placeholder="£50" class="shopping-2-input">
    <button data-donation="50" class="donate-button shopping-2-button">donate now</button>
</div>
    
<div class="shopping-item">
    <p><span>£188</span> could help a family find and keep a home</p>
    <input type="number" name="amount" placeholder="£188" class="shopping-3-input">
    <button data-donation="188" class="donate-button shopping-3-button">donate now</button>
</div>
epascarello
  • 204,599
  • 20
  • 195
  • 236
0

You've got the selector right already, i.e. var buttons = document.querySelectorAll("button[data-donation]") is pulling in all the buttons having data-donation, but there's no need to do another querySelectorAll inside your loop, you just need to reference each button in your result with buttons[i].

Your other mistake is setting onclick = console.log(), instead of onclick = function() { console.log() }. So all you need to change to get it working is:

var buttons = document.querySelectorAll("button[data-donation]");

for (i = 0; i < buttons.length; i++) {
    var button = buttons[i];
    buttons[i].onclick = function() { console.log(button) };
}

http://jsfiddle.net/w6gofb30/4/

stovroz
  • 6,835
  • 2
  • 48
  • 59
0
document.querySelectorAll("button[data-donation]").every(function(e,i,a){
    e.addEventListener("click", function(){
        console.log(this);
        console.log(this.getAttribute("data-donation"));
    }, false);
});

but before this u need:

NodeList.prototype.every = function(f){
    for(var k=0;k<this.length;k++)
        f(this[k],k,this);
}

http://jsfiddle.net/sa0ezut7/

Guja1501
  • 999
  • 7
  • 16