jQuery code refactoring help needed

Question

An update to before, here's what I'm dealing with:

<body>
<div class="header"> <img class="imgLogo" src="img/vegtablelogo.jpg"> </div>
<div id="thumbsContainer">
  <div class="thumb" id="carrotThumb"> <img id="showCarrot" class="imgThumb" src="img/carot.jpg" onClick=setupVeg("showCarrot", "carrotBig") /> </div>
  <div class="hidden" id="carrotBig"> <img class="imgBig" src="img/carot.jpg" /> </div>
  <div class="thumb" id="brocThumb"> <img id="showBroc" class="imgThumb" src="img/brocoli.jpg" onClick=setupVeg("showBroc", "brocBig") /> </div>
  <div class="hidden" id="brocBig"> <img class="imgBig" src="img/brocoli.jpg" /> </div>
</div>
<!-- end thumbs container --> 

<script>
var active = "";

function setupVeg(thumbVeg, hiddenVeg) {
    $("#" + thumbVeg).click(function() {
        if (active != hiddenVeg) {
            $("div.hidden").hide("fast");
            $("#" + hiddenVeg).show("fast", function() {});
            active = hiddenVeg;
        }
        else {
            $("div.hidden").hide("fast");
            active="";
        }
    });
}

$("div.hidden").click(function () {
    $("div.hidden").hide("fast");
    isAnyBig=false;
});

</script>
</body>

This code is not working unfortunately. I have borrowed from suggested solution below.

Would be nice if it did work! Any suggestions, most welcome.

seeing your code one improving you can use is definetly jquery's toggle method api.jquery.com/toggle — kobe
– kobe, Commented Dec 3, 2010 at 1:12

David Tang · Accepted Answer · 2010-12-03 01:18:32Z

3

I don't think you need any of the flags or the if conditions really. I think your logic is:

toggle carrotBig whenever showCarrot is clicked.
hide div.hidden whenever showCarrot is clicked.

So all you need is:

$("#showCarrot").click(function () {
   $("#carrotBig").toggle("fast");
   $("#div.hidden").hide();
});

.toggle will handle one of your flags (isCarrotBig) and .hide() won't do anything if div.hidden is already hidden, so that takes care of your isAnyBig flag.

Now.. let's make that work with broc as well...

function setupVegetable(showId, toggleId) {
   $("#" + showId).click(function () {
      $("#" + toggleId).toggle("fast");
      $("#div.hidden").hide();
   });
}

setupVegetable("showCarrot", "carrotBig");
setupVegetable("showBroc", "brocBig");

If you're interested, you can refactor it FURTHER so you don't need to supply the IDs for each of the vegetables. I'll need to see your HTML markup though.

answered Dec 3, 2010 at 1:18

David Tang

93.8k32 gold badges169 silver badges149 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Alex Over a year ago

But surely if I then display carrotBig and then perform $("#div.hidden").hide() immediately after, then it will force the large picture to suddenly vanish after clicking.?

Alex Over a year ago

my carrotBig equivalent is part of the <div class="hidden"> you see.

Alex Over a year ago

I have updated the question. I'm now working within my own function that is activated, but the code isn't working. It's late and I'll have to come back to it :)

David Tang · Accepted Answer · 2010-12-03 03:21:20Z

1

Ok I'll post a new answer in response to the edit.

Points worth noting:

Removed divs surrounding the imgs - they are unnecessary and complicate the relationship between the thumnnails and the large images.
Removed onclick attribute from within HTML - you will be attaching the event handlers in the JS so this is not needed.
Since the relationship between the thumbnails and the large images is quite obvious (the large images is just the next element) you don't need IDs to identify ANY of them. All you need is a class on the thumbnails.
Since we're not using IDs, only classes, you can add as many vegetables as you want without touching the JS

Your code modified:

<body>
<div class="header"> <img class="imgLogo" src="img/vegtablelogo.jpg"> </div>
<div id="thumbsContainer">
  <img class="imgThumb" src="img/carot.jpg" /> 
  <img class="imgBig hidden" src="img/carot.jpg" />
  <img class="imgThumb" src="img/brocoli.jpg" />
  <img class="imgBig hidden" src="img/brocoli.jpg" />
</div>
<!-- end thumbs container --> 

<script>

$("#thumbsContainer .imgThumb").click(function () {
   var thisImgBig = $(this).next();

   // Hide all imgBigs, except for this one
   $("#thumbsContainer .imgBig").not(thisImgBig[0]).hide();
   // Toggle this imgBig
   thisImgBig.toggle();
});

$("#thumbsContainer .imgBig").click(function () {
   // Hide this imgBig
   $(this).hide();
});

</script>
</body>

edited Dec 3, 2010 at 3:21

answered Dec 3, 2010 at 3:13

David Tang

93.8k32 gold badges169 silver badges149 bronze badges

2 Comments

Alex Over a year ago

Ok, this does get close to what I'm after... problem is that it shifts the thumbs around the page when clicked and doesn't do any layering over. For this reason I need to use DIVs. Really appreciate the effort though - good tips also. Thanks.

David Tang Over a year ago

I think you just need to apply the same CSS you have for the divs on the imgs. If you didn't have CSS for positioning, then I think it's because divs are block elements while imgs are inline. So just set style="display:block" on the imgs. If all that fails, and you really need the HTML you had before, then you can use similar methods as .next(), but have a look into .parent() - api.jquery.com/parent - and .children() - api.jquery.com/children. Glad to be of help.

hvgotcodes · Accepted Answer · 2010-12-03 01:13:47Z

0

create a function and reuse it....something like:

/**
 * document here....
 */
var toggleElements = function() {
// your code here
}

and then

$("#whatever").click(toggleElements);

answered Dec 3, 2010 at 1:13

hvgotcodes

121k33 gold badges209 silver badges237 bronze badges

Comments

Swizec Teller · Accepted Answer · 2010-12-03 01:18:22Z

Personally I would suggest creating a simple jQuery plugin. Something like so:

(function($){
 $.fn.big = function(options) {

  var defaults = {
   target: '#carrotBig',
  };
  var options = $.extend(defaults, options);

  return this.each(function() {
$(this).click(function () {
    isBrocBig=false;
  if (isCarrotBig == false && isAnyBig == false) {
    $(options.target).show("fast", function() {});
    isCarrotBig=true;
    isAnyBig=true;
    }
  else if (isCarrotBig == true) {
    $(options.target).hide("fast");
    isCarrotBig=false;
    isAnyBig=false;
  }
  else if (isCarrotBig == false && isAnyBig == true) {
    $("div.hidden").hide("fast");
    $(options.target).show("fast", function() {});
    isCarrotBig=true;
  }
  else {
      $("div.hidden").hide("fast");
      isCarrotBig=false;
      isAnyBig=false;
  }
});
  });
 };
})(jQuery);

Then you just call it with something like so:

 $("#showCarrot").big({target: '#carrotBig'})

Your next step should be to investigate whether you can get rid of the global variables or not.

Alex · Accepted Answer · 2010-12-03 11:37:52Z

Ok I have found a neat(ish) sollution, dependent on each hidden DIV being the .next() one. If it isn't it won't work but should be fine generally though. Hacked!

<div class="header"> <img class="imgLogo" src="img/vegtablelogo.jpg"> </div>
<div id="thumbsContainer">
  <div class="thumb" id="carrotThumb"> <img id="showCarrot" class="imgThumb" src="img/carot.jpg" /> </div>
  <div class="hidden" id="carrotBig"> <img class="imgBig" src="img/carot.jpg" /> </div>
  <div class="thumb" id="brocThumb"> <img id="showBroc" class="imgThumb" src="img/brocoli.jpg" /> </div>
  <div class="hidden" id="brocBig"> <img class="imgBig" src="img/brocoli.jpg" /> </div>
</div>
<!-- end thumbs container --> 

<script>
var active = "";

$("div.thumb").click(function() {
    var thumbVeg = $(this).attr("id");
    var hiddenVeg = $(this).next().attr("id");
    setupVeg(thumbVeg, hiddenVeg);
});



function setupVeg(thumbVeg, hiddenVeg) {
    if (active != hiddenVeg) {
        $("div.hidden").hide("fast");
        $("#" + hiddenVeg).show("fast", function() {});
        active = hiddenVeg;
    }
    else {
        $("div.hidden").hide("fast");
        active="";
    }
}

$("div.hidden").click(function () {
    $("div.hidden").hide("fast");
});

</script>

Collectives™ on Stack Overflow

jQuery code refactoring help needed

5 Answers 5

3 Comments

2 Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

3 Comments

2 Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related