1
\$\begingroup\$

I have two groups of two buttons (status). If you click on "Complete" or "On Hold", it will change the background color of the button. I am using same class name for all the buttons but each group of buttons have data attribute data-section='Park One' and data-section='Park Two'

jsFiddle

I am using the $.each() function to detect the section, remove all the background and add the background-color on selected button. Is the code well coded? What can be improved?

jQuery:

$(".updateStatus").click(function () {

    var buttonValue = $(this).val();
    var section = $(this).data("section");

    var cssClass = (buttonValue === "complete") ? 'green' : 'orange';

    $('.updateStatus').each(function () {
        if ($(this).data("section") == section) {

            $(this).removeClass("green orange");

            if ($(this).val() == buttonValue) {
                $(this).addClass(cssClass);
            }
        }
    })

});

HTML

<h2>Park One</h2>

<button class="updateStatus" value="complete" data-section='Park One'>Complete</button>
<button class="updateStatus" value="on hold" data-section='Park One'>On Hold</button>

<h2>Park Two</h2>

<button class="updateStatus" value="complete" data-section='Park Two'>Complete</button>
<button class="updateStatus" value="on hold" data-section='Park Two'>On Hold</button>
\$\endgroup\$
  • \$\begingroup\$ I was originally writing a write-up for this but I figured I'd ask this first: is there any reason you're using javascript/jquery for this instead of radio buttons? \$\endgroup\$ – Dan Pantry Jul 11 '15 at 0:01
  • \$\begingroup\$ @DanPantry Because in real application, I am using like Button Group, Example: getbootstrap.com/components/#btn-groups-single \$\endgroup\$ – I'll-Be-Back Jul 11 '15 at 0:03
  • \$\begingroup\$ Semantically speaking, radio buttons appear to make more sense here - there's no need for JavaScript. Bootstrap's styles/classes can be applied to any element, not just buttons. Heck, the site you've linked says that all you need to do is make your 'buttons' have the class .btn - not that they actually have to be buttons. \$\endgroup\$ – Dan Pantry Jul 11 '15 at 0:06
  • \$\begingroup\$ However I appreciate you asked for a review of your JavaScript, so I'm not going to post this as an answer, it's just an alternative approach. \$\endgroup\$ – Dan Pantry Jul 11 '15 at 0:07
  • \$\begingroup\$ The only issue I see is that you lookup your DOM to much. You should save the selectors to a variable var $this = $(this) and var $updateStatus = $('.updateStatus') \$\endgroup\$ – Dieterg Jul 11 '15 at 7:05
2
\$\begingroup\$

If you must use jQuery, then the simplest way by far is to wrap each section in a div :

<div class="section">
    <h2>Park One</h2>
    <button class="updateStatus" value="complete">Complete</button>
    <button class="updateStatus" value="on hold">On Hold</button>
</div>

<div class="section">
    <h2>Park Two</h2>
    <button class="updateStatus" value="complete">Complete</button>
    <button class="updateStatus" value="on hold">On Hold</button>
</div>

This allows you to use .siblings("button") to select the clicked button's counterpart :

$(".updateStatus").click(function () {
    $(this)
        .addClass(($(this).val() === "complete") ? 'green' : 'orange')
        .siblings("button").removeClass("green orange");
});

The .each() loop vanishes.

It's very common to use divs or spans like this, to give your DOM structure.

Updated fiddle

\$\endgroup\$
0
\$\begingroup\$

I suggest a simplified version, based on the following reasoning:

  • the first idea is to reduce the DOM lookup process; this can be achieved using a different method to target the involved buttons.
    Instead of selecting all buttons of the updateStatus class and so having to loop with $('.updateStatus').each(), we can select only the buttons of the clicked class: $('[data-section="' + this.dataset.section + '"]') to remove the color classes on them
  • in the other hand, for affecting the button color, we can add a data-color class to each button: this way each one contains its own color, so the affectation becomes direct and independant of the context, as soon as the button has been clicked.
    BTW there is no need to recognize the button by its value, so the valueattribute can be dropped.

The actual result is a reduced code, like this (see also this fiddle):

JS

$(".updateStatus").click(function () {
    $('[data-section="' + this.dataset.section + '"]')
        .removeClass('green orange');
    $(this).addClass(this.dataset.color);
});

Note that for getting data-* values we're using pure JS rather than jQuery: when jQuery doesn't bring an advantage, it's always a better choice from a performance point of view, because it uses a layer less.

HTML

<h2>Park One</h2>

<button class="updateStatus" data-section='Park One' data-color="green">Complete</button>
<button class="updateStatus" data-section='Park One' data-color="orange">On Hold</button>

<h2>Park Two</h2>

<button class="updateStatus" data-section='Park Two' data-color="green">Complete</button>
<button class="updateStatus" data-section='Park Two' data-color="orange">On Hold</button>
\$\endgroup\$

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.