current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

So I know close to nothing about jQuery - but I swear I'm trying to learn. The thing is, I've got some code that works - but I know it's repetitive and probably not kosher for a real programmer - which is why I've turned to you all.

So what I want to do is show/hide (or toggle - whatever you think is best) some informational divs, or so you might call them, on this page: Click for some pretty darn bad jQuery coding

Anyway, the show/hide code that I have right now stands at this: 

    $(document).ready(function(){

    $('#meet_growlab, #buddy_tv').hide();

    $('a#growlab').click(function(){
    $('#meet_growlab').show('slow');
});

    $('a#growlab_close').click(function(){
    $('#meet_growlab').hide('slow');
})

    $('a#buddytv').click(function(){
    $('#buddy_tv').show('slow');
});

    $('a#buddytv_close').click(function(){
    $('#buddy_tv').hide('slow');
})


});

With the HTML being (well the gist of it being...): 

<div id="meet_growlab">BLAH BLAH BLAH
<p><a href="#" id="growlab_close">Close</a></p>
</div>

<div id="buddy_tv">BLAH BLAH BLAH
<p><a href="#" id="buddytv_close">Close</a></p>
</div>

<ul>
    <li><a href="#" id="growlab" rel="#meet_growlab">Meet GrowLab - Canada’s Y-Combinator Arrives in Vancouver (June 24, 2011)</a></li>
    <li><a href="#" id="buddytv" rel="#buddy_tv">Building the Web's Best Entertainment-Based Community Site: Andy Liu, CEO and Founder of BuddyTV (April 1, 2011)</a></li>
</ul>

So yeah - it works, but it's not pretty. And I know you guys can help me make it pretty, so...will you? Thanks in advance!

share|improve this question

1 Answer 1

up vote 1 down vote
  1. You could have the hide/show divs inside a container, and then on .ready hide all its immediate children;
  2. You could then target all a.close inside this container, and on click hide the clicked anchor's grand-grandparent;
  3. And finally target all anchors inside the items list, and on click show the div corresponding to the anchor's rel;
  4. I would also make the anchor's href the same as its rel, so that if any user has JavaScript disabled the anchor still works.

So, HTML:

<div id="eventDescriptions">
  <div id="meet_growlab">BLAH BLAH BLAH
    <p><a href="#growlab" class="close">Close</a></p>
  </div>
  <div id="buddy_tv">BLAH BLAH BLAH
    <p><a href="#buddytv" class="close">Close</a></p>
  </div>
</div>
<ul id="eventTitles">
    <li><a href="#" id="growlab" rel="#meet_growlab">Meet GrowLab - Canada’s Y-Combinator Arrives in Vancouver (June 24, 2011)</a></li>
    <li><a href="#" id="buddytv" rel="#buddy_tv">Building the Web's Best Entertainment-Based Community Site: Andy Liu, CEO and Founder of BuddyTV (April 1, 2011)</a></li>
</ul>

and a more compact and DRY JQuery:

$(document).ready(function(){
  $('#eventDescriptions>div').hide();
  $('#eventTitles a').click(function(){
    var target = $(this).attr("rel");
    $(target).show('slow');
  });

  $('#eventDescriptions .close').click(function(){
    $(this).parent().parent().hide('slow');
  })
});

Here, a JSFiddle: http://jsfiddle.net/QYjLY/

Other things I quickly noticed:

  1. Good job on using rel;
  2. Instead of <img border=0 ... you could use CSS - a img { border: 0;};
  3. #social is a list, so it could be a ul instead of a div.
share|improve this answer
    
Thanks for the tips ANeves! And as for #social being a list - even when it's not incorporating the li tag to bring about different elements it's still considered a list? I thought it was just a div because it's a bunch of images, and not text... –  Connie Oct 25 '11 at 21:50
    
I should probably be more eloquent in my question, namely if I'm displaying a set of images, one after another in a horizontal line, that's a list? Isn't it just more code to add a ul component? I have to have div anyway for the social media buttons to position it correct relative to the nav menu and sub menu and the news ticker. Plus those images aren't really even a list - just a conglomeration of...well, images. –  Connie Oct 25 '11 at 22:11
    
I think I should have used could instead of the strong should. My thought was that abstractly it is a list of... other presences on the web? So it could be a ul, then styled to be shown in a line. Not too important, though. –  ANeves Oct 26 '11 at 9:44
    
Aha. I get it. Thanks for the clarification and thanks for all the help! –  Connie Oct 26 '11 at 20:12

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.