4
\$\begingroup\$

This question is about an improvement of the code from this previous question (previous changed visibility, new applies a CSS class with a border).

It is a constructor function that attaches a click event to radio inputs using event delegation in order to add, remove CSS classes and enable the text input which contains a data-value equals to the radio value.

Short example

<label><input type="radio" name="color" value="yellow" checked />Yellow</label>
<input type="text" class="radio-condition visible" data-value="yellow" />

One important fact is that it must work for old browsers such as IE7-8. I know I'm using methods from EcmaScript 6 (is ES6 right?) such as classList which won't work. But I found this link from hacks.Mozilla.org where it shares a code for older browsers. So I guess I can use those ES6 methods safely, right?

jsFiddle here!

<script src="ie8_conditional_comments_with_script_from_hacks.Mozilla.js"></script>

<form>
    <div id="radio-set" class="radio-set">
<label><input type="radio" name="color" value="yellow" checked />Yellow</label>
<label><input type="radio" name="color" value="brown" />Brown</label>
<label><input type="radio" name="color" value="orange" />Orange</label>
<label><input type="radio" name="color" value="purple" />Purple</label>

<input type="text" class="radio-condition visible" placeholder="Which yellow?" data-value="yellow" />
<input type="text" class="radio-condition" placeholder="Which brown?" data-value="brown" disabled />
<input type="text" class="radio-condition" placeholder="Which Orange?" data-value="orange" disabled />
<input type="text" class="radio-condition" placeholder="Which Purple?" data-value="purple" disabled />

</div>

</form>

<style>
    .radio-condition {border: 1px solid red; display: block; margin-bottom: 2px}
    .visible {border-color: green}
</style>

<script>
var JavaScript_form = function() {
//  this.radio_set = document.getElementById('radio-set'),
  this.radio_set = document.querySelector('radio-set'),
  this.radios = document.querySelectorAll('input[type="radio"]');
}

// 
JavaScript_form.prototype.radio_hide_element = function() {

  // hide elements
  var els_hidden = document.getElementsByClassName('radio-condition'),
      els_hidden_length = els_hidden.length;

  // Event handler that uses event delegation
  document.getElementById('radio-set').addEventListener('click', function(e) {
    // e.target was the clicked element
    if (e.target && e.target.nodeName == 'INPUT' && e.target.getAttribute('type') == 'radio') {
      console.log(e.target.value); 
      var inputs = document.querySelectorAll('.radio-condition'),
          i = 0,
          inputs_length = inputs.length;

      for (; i < inputs_length; i++) {
        inputs[i].classList.remove('visible');
        inputs[i].disabled = true;
      }

      document.querySelector("[data-value='" + e.target.value + "']").className += ' visible';
      document.querySelector("[data-value='" + e.target.value + "']").disabled = false;
    }
  });
}
var JS_radios = new JavaScript_form();
JS_radios.radio_hide_element();
     </script>
\$\endgroup\$
  • \$\begingroup\$ What exactly is the question? \$\endgroup\$ – jfriend00 Aug 27 '14 at 4:34
  • 4
    \$\begingroup\$ @jfriend00 I just ask for a review. \$\endgroup\$ – Ricardo Castañeda Aug 27 '14 at 4:35
  • 1
    \$\begingroup\$ Gah! Underscores in JavaScript! Underscores in JavaScript!! MY EYES!! All jokes aside, I'm writing a review right now :P \$\endgroup\$ – Madara Uchiha Aug 30 '14 at 12:41
3
\$\begingroup\$

Let's start with a dry review:

  • Underscores in JavaScript hurt the eyes. The absolute naming convention of JavaScript is to use camelCase. Your underscore notation, while consistent, hurts the eyes of other JavaScript developers.
  • Using event delegation is great, but since you've queried the DOM once when you instantiated the element, you can only use the elements which existed in the DOM when you instantiated the object. Boo.
  • This line

      this.radio_set = document.querySelector('radio-set'),
    

    selects the first <radio-set></radio-set> element.

  • You're selecting all of the radio buttons on init, what happens if you have other radio elements which aren't related to your module? You'll process them too, which is undesirable.


Now a bit on the approach, I prefer the Revealing Module Pattern for making self-contained modules. It allows you to have public and private properties and methods, and only reveal what you want.

My Take

jsFiddle

 <form id="module">
    <label><input type="radio" name="color" value="yellow" checked/>Yellow</label>
    <label><input type="radio" name="color" value="brown"/>Brown</label>
    <label><input type="radio" name="color" value="orange"/>Orange</label>
    <label><input type="radio" name="color" value="purple"/>Purple</label>

    <input type="text" class="radio-condition visible" placeholder="Which yellow?" data-value="yellow"/>
    <input type="text" class="radio-condition" placeholder="Which brown?" data-value="brown" disabled/>
    <input type="text" class="radio-condition" placeholder="Which Orange?" data-value="orange" disabled/>
    <input type="text" class="radio-condition" placeholder="Which Purple?" data-value="purple" disabled/>
</form>

<script>
    //Revealing Module Pattern, this function calls itself!
    var myModule = (function (target) { //Accept a target parameter. Everything is relative to target.

        var init = function() { //This function is public, it gets returned at the end
            target.onclick = function(e) { //Event delegation, look for clicks on target
                if (e.target.type == "radio") { //If clicked target is a radio
                    var matchingInput = target.querySelector('input[type="text"][data-value="'+ e.target.value +'"]');
                    switchToInput(matchingInput); //Switch to the matching input there.
                }
            };
        };

        var switchToInput = function(input) { //This method doesn't get returned. It's not accessible outside.
            var allTextInputs = target.querySelectorAll('input[type="text"]');
            //allTextInputs is not an array. So I can't use allTextInputs.forEach().
            //[].forEach.call is a shorthand for Array.prototype.forEach.call, to call
            //the forEach on an iterable object, which isn't an array.
            [].forEach.call(allTextInputs, function(input) { //Disable all inputs
                input.disabled = true;
                input.classList.remove("visible");
            });

            input.disabled = false; //Then enable the one passed in.
            input.classList.add("visible");
        };

        //Reveal the init function, making it public.
        return {
            init: init
        };
    })(document.getElementById("module")); //Pass in our parent, in this case, the form#module.

    myModule.init(); //Run the init function.

</script>
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks a lot @MadaraUchiha, I really appreciate the time you dedicated to this review and your solution. \$\endgroup\$ – Ricardo Castañeda Aug 31 '14 at 21:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.