CODEWITHSUNDEEP
javascriptjqueryrefactoring
Alex
Alex

Reputation: 4934

jQuery code refactoring help needed

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.

Upvotes: 1

Views: 132

Answers (5)

Alex
Alex

Reputation: 4934

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>

Upvotes: 0

David Tang
David Tang

Reputation: 93664

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>

Upvotes: 1

David Tang
David Tang

Reputation: 93664

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.

Upvotes: 3

Swizec Teller
Swizec Teller

Reputation: 2322

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.

Upvotes: 0

hvgotcodes
hvgotcodes

Reputation: 120178

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

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

and then

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

Upvotes: 0

Related Questions