CODEWITHSUNDEEP
jquery
Kelsey
Kelsey

Reputation: 47726

Can I improve this JQuery image replace code?

The HTML looks kind of like:

<dl>
  <dt>
    <img src='Images\Something\expand-close.gif' /> Something 1
  </dt>
  <dd>Something 1 Text</dd>
</dl>

This HTML is repeated 1 or more times so there could be many instances of the HTML on the same page.

The Jquery I am using to expand the dd and replace the image is:

$("dl").find("dt").click(function() {
  // toggle the dd
  $(this).next().toggle(200);

  // replace the expand / close image
  var img = $(this).find("img");
  var src = img.attr("src");
  if (src.lastIndexOf("open.gif") != -1)
  {
    img.attr("src", src.replace("open.gif", "closed.gif"));
  }
  else
  {
    img.attr("src", src.replace("closed.gif", "open.gif"));
  }
});

This all works fine and does exactly what I need it to do. My question is, can the JQuery function be done better? I am relatively new to using JQuery and this one was a snippet I rattled off. If it could be done better, please explain the changes as I am trying to learn how to write better JQuery code. Any other pointers or suggestions would be appreciated as well.

Upvotes: 4

Views: 3641

Answers (4)

farinspace
farinspace

Reputation: 8801

You can play around with the settings but here is the general idea, a combination of CSS and JS ...

<style>

    dl dt { background: transparent url(open.gif) no-repeat scroll top left; padding-left: 20px; }
    .open dt { background-image: url(close.gif); }

</style>

<dl class="open">
    <dt>Something 1</dt>
    <dd>Something 1 Text</dd>
</dl>

<script>

    $('dl dt').click(function()
    {
        $(this).next().toggle(200);
        $(this).parent().toggleClass("open",$('dd:visible',$(this).parent()).length);
    });

</script>

Upvotes: 1

cletus
cletus

Reputation: 625077

It's pretty close. Try:

$("dl > dt").click(function() {
  var dd = $(this).next();
  if (dd.is(":visible")) {
    var newimage = "closed.gif";
    dd.hide(200);
  } else {
    var newimage = "open.gif";
    dd.show(200);
  }
  var img = $(this).find("img");
  img.attr("src", img.attr("src").replace(/(open|closed)\.gif$/, newimage);
});

The differences are:

  1. Just use "dl > dt" (or "dl dt" as appropriate) instead of the find syntax in that situation;
  2. This code looks at if the thing is visible or not rather than the src image, which could potentially get out of sync especially given the delay;
  3. The regex on the last line just replaces whatever is there with whatever is correct to cater for irregularities.

Upvotes: 3

chaos
chaos

Reputation: 124297

<dl>
  <dt class="something">
    <img class="switch" src='Images\Something\expand-close.gif' /> Something 1
  </dt>
  <dd>Something 1 Text</dd>
</dl>

$('.something').click(function() {
    // toggle the dd
    $(this).next().toggle(200);
    // replace the expand / close image
    var img = $(this).find('.switch');
    var src = img.attr('src');
    if(src.lastIndexOf('open.gif') != -1)
        img.attr('src', src.replace('open.gif', 'closed.gif'));
    else
        img.attr('src', src.replace('closed.gif', 'open.gif'));
});

Upvotes: 0

Jamie Rumbelow
Jamie Rumbelow

Reputation: 5095

You can set an ID or Class attribute to the image tag, which will clear up your code a bit. I'd also recommend using a plugin - there are plenty of plugins for this sort of thing available online for free, and all it requires is one method.

HTML:

<dl>
  <dt>
    <img src='Images\Something\expand-close.gif' id='myimage' /> Something 1
</dt>
  <dd>Something 1 Text</dd>
</dl>

JavaScript (using jQuery):

$('#myimage').click(function(){
  $(this).parent().get(0).toggle(200);
  var src = $(this).attr('src');

  if (src.lastIndexOf("open.gif") != -1) {
    $(this).attr('src', src.replace("open.gif", "closed.gif");
  } else {
    $(this).attr('src', src.replace("closed.gif", "open.gif");
  }
});

Hope that helps a bit :)

Upvotes: 2

Related Questions