Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

Okay, I'm under the impression that my current jQuery can be simplified (don't worry! It's short!) and I'm curious to know how this can be achieved. I think a solution to this will help produce a lot less code.

I currently have a list menu set up, with class names referring to their titles (HTML below) and if you click a list item, the menu item slides out and it's relevant content fades in. If you click the main heading again, the content fades out and the menu item slides back in.

Here's an example of my HTML

<ul class="wwd-filters">
   <li><a class="corporate"><span>Corporate</span></a></li>
   <li><a class="commercial"><span>Commercial</span></a></li>
   <li><a class="construction"><span>Construction</span></a></li>
</ul>

<div class="wwd-content-each corporate">
    <h2>Corporate</h2>
    <div class="wwd-slides">
    <div class="slides_container">
        <p>Lorem ipsum lorem ipsum lorem ipsum</p>                      
        </div>
    </div>
</div>
<div class="wwd-content-each commercial">
    <h2>Commercial</h2>
    <div class="wwd-slides">
    <div class="slides_container">
        <p>Lorem ipsum lorem ipsum lorem ipsum</p>                      
        </div>
    </div>
</div>

Here's some example JS:

jQuery('.wwd-choices-container ul.wwd-filters li a.corporate').click(function () {
    jQuery('.wwd-content-container .wwd-content-each').fadeOut();
    jQuery('.wwd-content-container .wwd-content-each.corporate').fadeIn();
});

My problem is that I don't want to have to write out the jQuery code X number of times for each list item and it's content. As the class names for both the list item anchor and each content, is there an easy way to simplify this?

Everything is built via a CMS, so new sections and menu items would be added as time goes on so it has to remain as dynamic as possible. I'm thinking some form of array would work... and if it matches each other then it connections.

Thanks in advance, R

share|improve this question
 
Does all your code work as intended? –  Andrew Peacock Jun 6 at 15:01
 
Yep, so what I have works — but I have to repeat the JS code for each item... which I felt could be simplified with some help. –  rdck Jun 6 at 15:03
 
If it was possible, what it would do is on clicking the anchor, get the class name, and only show the content below with the same class. –  rdck Jun 6 at 15:06
 
Since the code already works, this question belongs on Code Review. –  Michael Myers Jun 6 at 23:14

migrated from stackoverflow.com Jun 6 at 23:14

This question came from our site for professional and enthusiast programmers.

2 Answers

up vote 1 down vote

I would recommend using the .on() for click event delegation:

$('.wwd-filters').on('click', 'a', function(e) {
    e.preventDefault();
    var id = $(this).data('id');
    jQuery('.wwd-content-container .wwd-content-each').fadeOut();
    jQuery('.wwd-content-container .wwd-content-each.' + id).fadeIn();
});

This implementation grabs the id of the section you wish to show via the data attribute in the a elements:

<ul class="wwd-filters">
   <li><a data-id="corporate" href="#corporate"><span>Corporate</span></a></li>
   <li><a data-id="commercial" href="#commercial"><span>Commercial</span></a></li>
   <li><a data-id="construction" href="#construction"><span>Construction</span></a></li>
</ul>
share|improve this answer
 
Many thanks for the help! This doesn't seem to do it just yet, although I can't see why? Quick JS: jsfiddle.net/uR9nU/1 –  rdck Jun 6 at 15:23
1  
Think you need to tweak the chaining .find() - It won't find .corporate because it is the same element as .wwd-content-each. –  Nick Jun 6 at 15:24
1  
@Nick - You're correct. I've updated my answer accordingly. –  adamb Jun 6 at 15:32
up vote 0 down vote

In the click handler you have a reference to the link that was clicked, so you can just select the relevant item from this. Here I've used the href of the link to point at an ID for each item, which makes it simple to do jQuery(this.hef) to select the item to fade in. It might not be convenient for you to use the ID element, in which case you can put them in a data attribute or something.

<ul class="wwd-filters">
   <li><a href="#corporate"><span>Corporate</span></a></li>
   <li><a href="#commercial"><span>Commercial</span></a></li>
   <li><a href="#construction"><span>Construction</span></a></li>
</ul>

<div class="wwd-content-each" id="corporate">
    <h2>Corporate</h2>
    <div class="wwd-slides">
    <div class="slides_container">
        <p>Lorem ipsum lorem ipsum lorem ipsum</p>                      
        </div>
    </div>
</div>
<div class="wwd-content-each" id="commercial">
    <h2>Commercial</h2>
    <div class="wwd-slides">
    <div class="slides_container">
        <p>Lorem ipsum lorem ipsum lorem ipsum</p>                      
        </div>
    </div>
</div>

Then you should just need this one snippet:

jQuery('.wwd-choices-container ul.wwd-filters li a').click(function () {
    jQuery('.wwd-content-container .wwd-content-each').fadeOut();
    jQuery(this.href).fadeIn();
    return false;
});
share|improve this answer
 
You can do this using the classes as in your example, it's just that you have to be a lot more careful since you might add other classes to your links for other purposes, and then parsing that becomes a bit of a pain (how does it know which class is the identifier for the section to fade in, etc.) –  Nick Jun 6 at 15:09
 
Thanks, Nick — this is making perfect sense. I guess we could be more explicit about the class names? So include .wwd-content-each#corporate etc. Would that work? –  rdck Jun 6 at 15:11
 
As #corporate is a (unique) ID, there's no need to qualify it further with .wwd-content-each. My comment was more about putting the target in a class attribute on the link you click. It would be best in its own attribute, which could be anything (a data-id as adamb has done, or the href as I've done). –  Nick Jun 6 at 15:18
2  
Here's an updated version with the fix to the comment mentioned in adamb's commented, plus you were missing the wwd-content-container that the script looks for - jsfiddle.net/uR9nU/2 - some CSS issues to work out but the script is doing its thing. –  Nick Jun 6 at 15:26
2  
Here you go, a bit of CSS to fix the positioning (gosh, I must be feeling helpful today!) jsfiddle.net/uR9nU/3 –  Nick Jun 6 at 15:28
show 2 more comments

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.