2

In an HTML form, I have the following section, which I want to act like radio buttons but appear like regular buttons.

<span>Minimum Skill Level:</span><br />
<input type='button' id='skMin1' class='btn-sm' name='skMin' value='1' checked>
<input type='button' id='skMin2' class='btn-sm' name='skMin' value='2'>
<input type='button' id='skMin3' class='btn-sm' name='skMin' value='3'>
<input type='button' id='skMin4' class='btn-sm' name='skMin' value='4'>
<input type='button' id='skMin5' class='btn-sm' name='skMin' value='5'>

By making the name the same for all of the buttons in the group, they act like radio buttons in that clicking on a second deselects the first.

I have a section of JavaScript which adds a listener to each button and saves the value. It changes the background color of the button to show that it's been selected.

let radioButton1 = document.querySelectorAll("[name=skMin]").forEach(function(e) {
        e.addEventListener('click', () => {
        e.style.background = 'cyan';
        drillSession.skillMin = e.value;
    })
})

It looks like I want, and I'm getting the data just fine, but there's one big problem. Clicking on a second button within the group does change the value, but I can't figure out how to unhighlight the previously selected option, so the user won't be confused by seeing multiple highlighted buttons. Any suggestions?

Yuda
  • 343
  • 4
  • 15
John Biddle
  • 2,664
  • 2
  • 15
  • 25
  • The way you're adding the listener creates a closure and circular reference to every button you put the function on, and means *this* is useless. Consider a separate function and adding a reference instead. That will avoid the above issues and you can use *this* instead of *e*. – RobG Jan 17 '19 at 01:26

3 Answers3

1

Try

let radioButton1 = document.querySelectorAll("[name=skMin]").forEach(function(e,i,a) {
    e.addEventListener('click', (event) => {
        a.forEach(x=>x.style.background = 'white')
        e.style.background = 'cyan'
        //drillSession.skillMin = e.value       
    })
})
<span>Minimum Skill Level:</span><br />
<input type='button' id='skMin1' class='btn-sm' name='skMin' value='1' checked>
<input type='button' id='skMin2' class='btn-sm' name='skMin' value='2'>
<input type='button' id='skMin3' class='btn-sm' name='skMin' value='3'>
<input type='button' id='skMin4' class='btn-sm' name='skMin' value='4'>
<input type='button' id='skMin5' class='btn-sm' name='skMin' value='5'>
Kamil Kiełczewski
  • 85,173
  • 29
  • 368
  • 345
  • What is the "i" for in forEach(function(e,i,a) { ... ? – John Biddle Jan 17 '19 at 02:33
  • @JohnBiddle as is written [here](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#Parameters) forEach callback function takes 3 parameters `function(element, index, array)` - `element` is current element, `index` is element index in array, and `array` is whole array - your example is first where I see that `array` is useful (but to get it we need to declare index i , however we don't use it) – Kamil Kiełczewski Jan 17 '19 at 02:42
1

Note that e in an event handler is often used to refer to the event parameter passed - to avoid confusion, you might consider using a different name, for example, button or skMinButton or something of the sort.

One option is to iterate over all buttons first, and remove their background property:

const buttons = document.querySelectorAll("[name=skMin]");
buttons.forEach((skMinButton) => {
  skMinButton.addEventListener('click', () => {
    buttons.forEach((button) => {
      button.style.removeProperty('background');
    });
    skMinButton.style.background = 'cyan'
    // drillSession.skillMin = e.value
  });
});
<span>Minimum Skill Level:</span><br />
<input type='button' id='skMin1' class='btn-sm' name='skMin' value='1' checked>
<input type='button' id='skMin2' class='btn-sm' name='skMin' value='2'>
<input type='button' id='skMin3' class='btn-sm' name='skMin' value='3'>
<input type='button' id='skMin4' class='btn-sm' name='skMin' value='4'>
<input type='button' id='skMin5' class='btn-sm' name='skMin' value='5'>

A more efficient possibility would be to save the previously selected button in a variable, and only remove its background:

const buttons = document.querySelectorAll("[name=skMin]");
let selectedButton;
buttons.forEach((skMinButton) => {
  skMinButton.addEventListener('click', () => {
    if (selectedButton) {
      selectedButton.style.removeProperty('background');
    }
    selectedButton = skMinButton;
    skMinButton.style.background = 'cyan'
    // drillSession.skillMin = e.value
  })
})
<span>Minimum Skill Level:</span><br />
<input type='button' id='skMin1' class='btn-sm' name='skMin' value='1' checked>
<input type='button' id='skMin2' class='btn-sm' name='skMin' value='2'>
<input type='button' id='skMin3' class='btn-sm' name='skMin' value='3'>
<input type='button' id='skMin4' class='btn-sm' name='skMin' value='4'>
<input type='button' id='skMin5' class='btn-sm' name='skMin' value='5'>

You also might consider using event delegation instead, so that only one listener is attached, rather than a separate one for each button:

let selectedButton;
document.querySelector('.min-skill').addEventListener('click', ({ target }) => {
  if (!target.matches('input')) {
    return;
  }
  if (selectedButton) {
    selectedButton.style.removeProperty('background');
  }
  selectedButton = target;
  target.style.background = 'cyan'

  // drillSession.skillMin = e.value
});
<div class="min-skill">
  <span>Minimum Skill Level:</span><br />
  <input type='button' id='skMin1' class='btn-sm' name='skMin' value='1' checked>
  <input type='button' id='skMin2' class='btn-sm' name='skMin' value='2'>
  <input type='button' id='skMin3' class='btn-sm' name='skMin' value='3'>
  <input type='button' id='skMin4' class='btn-sm' name='skMin' value='4'>
  <input type='button' id='skMin5' class='btn-sm' name='skMin' value='5'>
</div>
CertainPerformance
  • 356,069
  • 52
  • 309
  • 320
1

The way the OP code is written, it creates closures and circular references that can be hogs on memory. You might like to clean it up by making listeners discrete functions rather than anonymous function expressions. Also, attaching listeners as references makes this more useful.

One easy way to do the highlighting/unhighlighting is to add and remove a class, then you can use a selector to find the highlighted ones efficiently, e.g.

var utils = {
  toggleSelected: function(evt) {
    let hClass = 'btn_selected';
    document.querySelectorAll('.' + hClass).forEach(el => el.classList.remove(hClass));  
    this.classList.add(hClass);    
  }
};

window.addEventListener('DOMContentLoaded',function(){
  document.querySelectorAll('input.btn-sm').forEach(btn =>
    btn.addEventListener('click', utils.toggleSelected, false));
}, false);
.btn_selected {
  background-color: #ffbb99;
}
<span>Minimum Skill Level:</span><br/>
<input type='button' class='btn-sm' name='skMin' value='1'>
<input type='button' class='btn-sm' name='skMin' value='2'>
<input type='button' class='btn-sm' name='skMin' value='3'>
<input type='button' class='btn-sm' name='skMin' value='4'>
<input type='button' class='btn-sm' name='skMin' value='5'>

Radio buttons do this by default so maybe you should look at styled radio buttons instead of scripted buttons.

var utils = {
  toggleSelected: function(evt) {
    let tgt = this;
    let div = this.closest('div')
    let hClass = 'btn_selected';
    div.querySelectorAll('.' + hClass).forEach(el => el.classList.remove(hClass));
    this.classList.add(hClass);
  },
  removeAllSelected: function(evt) {
    let hClass = 'btn_selected';
    document.querySelectorAll('.' + hClass).forEach(el => el.classList.remove(hClass));

  }
};

window.addEventListener('DOMContentLoaded', function() {
  document.querySelectorAll('input.btn-sm').forEach(btn =>
    btn.addEventListener('click', utils.toggleSelected, false));
    document.getElementById('theReset').addEventListener('click', utils.removeAllSelected, false);
}, false);
.btn_selected {
  background-color: #ffbb99;
}
<fieldset><legend>Select some values&hellip;</legend>
<div id="set0">
  <span>Minimum Skill Level:</span><br>
  <input type='button' class='btn-sm' name='skMin' value='1'>
  <input type='button' class='btn-sm' name='skMin' value='2'>
  <input type='button' class='btn-sm' name='skMin' value='3'>
</div>
<div id="set1">
  <span>Minimum Education Level:</span><br>
  <input type='button' class='btn-sm' name='skMin' value='1'>
  <input type='button' class='btn-sm' name='skMin' value='2'>
  <input type='button' class='btn-sm' name='skMin' value='3'>
</div>
<div id="set2">
  <span>Minimum Confidence Level:</span><br>
  <input type='button' class='btn-sm' name='skMin' value='1'>
  <input type='button' class='btn-sm' name='skMin' value='2'>
  <input type='button' class='btn-sm' name='skMin' value='3'>
</div>
<button id="theReset">Reset All</button>
</fieldset>
RobG
  • 142,382
  • 31
  • 172
  • 209
  • I tried the styled radio buttons approach and was not able to get them looking like I wanted. I'm quite new to JS (You can probably tell) and I'm having some trouble understanding your answer, and how I would use it for up to 10 questions on a page. – John Biddle Jan 17 '19 at 02:59
  • @JohnBiddle—you can add the listener to a containing element and do the highlight from there. There is also a *closest* method that is pretty well supported, so from the target element you can to to the closest ancestor with a particular tag and limit the scope of *querySelector* to that (e.g. wrap each set of buttons in a div). I can add an example later when I have time. – RobG Jan 17 '19 at 03:22