CODEWITHSUNDEEP
jquery
madmatuk
madmatuk

Reputation: 127

Is there a better way to do this jQuery function

I am new to jQuery but i am really liking it.

I have some jQuery as below that works as expected, i am jsut wandering if there is a more effective less resource hungry method to do the same thing.

<script type="text/javascript">
    $(function() {
        $("select.openTime").change(function() {
            if ($(this).val() != 'Closed') {
                $(this).next(".hidden_element").css('display', 'block');
            }
            if ($(this).val() == 'Closed') {
                $(this).next(".hidden_element").css('display', 'none');
            }

        });
    });
</script>

Any suggestions welcome. Thanks M

Upvotes: 1

Views: 78

Answers (9)

Lukas
Lukas

Reputation: 3235

First off, less DOM parsing.

var state = $(this).val();
var $next = $(this).next(".hidden_element");

DOM access is your most resource heavy operation. Minimize reference/scanning the DOM as much as possible. Also, make it easier by adding unique ids rather than classes for each block if you can. Maybe this is not the design...

Second, use classes or the show()/hide() methods. 

if(state == "CLOSED") { 
  $next.hide();
  // or...
  $next.addClass('hidden');
} else
  $next.show();
  // or...
  $next.removeClass('hidden');
}

Note: this is simple enough to make a clean ternary statement:

(state == "CLOSED") ? $next.hide() : $next.show();

That will reduce your conditional logic a bit as well as reduce your calls to search the DOM.

The end result:

<script type="text/javascript">
    $(function() {
        $("select.openTime").change(function() {
            var state = $(this).val();
            var $next = $(this).next(".hidden_element");

            (state == "CLOSED") ? $next.hide() : $next.show();        
        });
    });
</script>

Upvotes: 1

isNaN1247
isNaN1247

Reputation: 18099

Suggestions

  1. Use an ID rather than a tag/class selector as the underlying getElementById is more efficient
  2. Rather than display block/none - why not try .slideToggle() or fadeToggle() - check these out at http://api.jquery.com/

Upvotes: 3

Kyle Sletten
Kyle Sletten

Reputation: 5413

You can do it in raw javascript. Just add the following script to the onchange of your select elements:

var current = this.nextSibling;
while(current){
    if(current.className='hidden_element'){
        if(this.value == 'Closed'){
            current.style.display = 'none';
        }else{
            current.style.display = 'block';
        }
        return;
    }else{
        current = current.nextSibling;
    }
}

Upvotes: 1

Orbling
Orbling

Reputation: 20602

Well you could use else for a start. Or better still, just use .toggle()

$(function() {
    $("select.openTime").change(function() {
        $(this).next(".hidden_element").toggle($(this).val() != 'Closed');
    });
});

Upvotes: 1

Oliver Spryn
Oliver Spryn

Reputation: 17348

Take a look at this:

<script type="text/javascript">
  $(function() {
    $("select.openTime").change(function() {
      var object = $(this);

      if (object.val() != 'Closed') {
          object.next(".hidden_element").css('display', 'block');
      } else {
          object.next(".hidden_element").css('display', 'none');
      }
    });
  });
</script>

Thank should lighten up your resources.

Hope that helps.

Upvotes: 0

DGM
DGM

Reputation: 26979

That shouldn't be very resource heavy. The only thing I would change is to use show() and hide() instead of the css() call.

$(this).next(".hidden_element").show();

Upvotes: 0

Fosco
Fosco

Reputation: 38526

I don't see what's so resource hungry about this.. I fixed your conditional though:

    <script type="text/javascript">
$(document).ready(function(){
    $("select.openTime").change(function() { 
        if ($(this).val() == 'Closed') {    
                    $(this).next(".hidden_element").css('display','none');
            } else {
                    $(this).next(".hidden_element").css('display','block');
            }       
    });
});

</script>

Also:
css('display','none'); can be changed to hide();
css('display','block'); can be changed to show();

Upvotes: 0

AlbertVo
AlbertVo

Reputation: 772

looks good to me, you could also use the built in show and hide functions which might make your code more readable.

Upvotes: 0

Mahesh Velaga
Mahesh Velaga

Reputation: 21971

The following solution is a little more efficient and more readable: 

$(function(){

    $("select.openTime").change(function() { 
        var $nextHiddenElement = $(this).next(".hidden_element")
        
        if ($(this).val() != 'Closed') {   
            $nextHiddenElement.show();
        }
        else {   
            $nextHiddenElement.hide();
        }
    });

});

Upvotes: 2

Related Questions