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.

I am hiding and fading in different content on the same page using jQuery to hide() and fadeIn() the content depending on which link is clicked.

It works how I want it to, but the way I've written the jQuery looks like it could be simplified.

$("#navItem1").on('click', function(){
    $('#content-wrap1').hide();
    $('#content-wrap2').hide();
    $('#content-wrap3').hide();
    $('#content-wrap4').hide();
    $('#content-wrap5').hide();
    $('#content-wrap1').fadeIn(1000);
});
$("#navItem2").on('click', function(){
    $('#content-wrap1').hide();
    $('#content-wrap2').hide();
    $('#content-wrap3').hide();
    $('#content-wrap4').hide();
    $('#content-wrap5').hide();
    $('#content-wrap2').fadeIn(1000);
});
$("#navItem3").on('click', function(){
    $('#content-wrap1').hide();
    $('#content-wrap2').hide();
    $('#content-wrap3').hide();
    $('#content-wrap4').hide();
    $('#content-wrap5').hide();
    $('#content-wrap3').fadeIn(1000);
});
$("#navItem4").on('click', function(){
    $('#content-wrap1').hide();
    $('#content-wrap2').hide();
    $('#content-wrap3').hide();
    $('#content-wrap4').hide();
    $('#content-wrap5').hide();
    $('#content-wrap4').fadeIn(1000);
});
$("#navItem5").on('click', function(){
    $('#content-wrap1').hide();
    $('#content-wrap2').hide();
    $('#content-wrap3').hide();
    $('#content-wrap4').hide();
    $('#content-wrap5').hide();
    $('#content-wrap5').fadeIn(1000);
});


function fadeInFirstContent(){
    $('#content-wrap1').fadeIn(1000);

}
fadeInFirstContent();

Here's a jfiddle

How can I rewrite the jQuery so it does exactly the same thing but in much less code?

share|improve this question
1  
For some reason all the current answers seem to want you to change the HTML or add random loops. There is no need. My answer here uses basic jQuery functionality to achieve all of this in just 5 lines: codereview.stackexchange.com/a/26236/24039. You'll benefit from the official jQuery interactive tutorial: try.jquery.com. –  James Donnelly May 16 '13 at 12:49
8  
@JamesDonnelly, the HTML should be changed to use classes instead of these repetitive [id]s. It makes the HTML more expressive of the fact that these elements obviously share a similar class. –  zzzzBov May 16 '13 at 13:57
1  
While @crm might be constrained and isn't in a position to update the HTML, I agree with zzzzBov that it's the best direction to take. –  Derek May 16 '13 at 14:56

9 Answers 9

up vote 29 down vote accepted

Perhaps change the HTML:

Navitem:

<span class="navItem" data-content-id="1"></span>

The content

<div id="content-wrap1" class="content-wrap"></div>

jQuery

$('.navItem').on('click',function(){
   $('.content-wrap').hide();
   $('#content-wrap'+$(this).data('content-id')).fadeIn(1000);
});
share|improve this answer
1  
This is much better than using a substring of the ID. Also, you’re missing a . in front of navItem –  minitech May 16 '13 at 20:03
    
This is better than the accepted answer, though I feel it would be cleaner with data attributes on the content-wrap elements instead of an ID. A selector like .content-wrap[data-wrapid=VAR] wouldn't be noticeably slower. Alternatively, simply use eq() to select the nth element. –  DisgruntledGoat May 16 '13 at 22:51

yes there is...there are many ways to simplify this.. i am doing it by using data attribute to your <a> tag...and adding same class to all the contents <div> .(i am using content here)...

HTML

<nav>
<!-- Start of side menu -->
<ul id="mainNav">
    <li class="mainNavItem"><a href="#" id="navItem1" data-content="content-wrap1">Home</a>
    </li>
    <li class="mainNavItem"><a href="#" id="navItem2" data-content="content-wrap2">Prices</a>
    </li>
    <li class="mainNavItem"><a href="#" id="navItem3" data-content="content-wrap3">Find us</a>
    </li>
    <li class="mainNavItem"><a href="#" id="navItem4" data-content="content-wrap4">Reviews</a>
    </li>
    <li class="mainNavItem"><a href="#" id="navItem5" data-content="content-wrap5">Gallery</a>
    </li>
   </ul>
</nav>
<!-- closes mainNav -->
<div id="content-wrap1" class="content">Home</div>
<div id="content-wrap2" class="content">Prices contents</div>
<div id="content-wrap3" class="content">find us contents</div>
<div id="content-wrap4" class="content">review contents</div>
<div id="content-wrap5" class="content">gallery contents</div>

jquery

$('ul#mainNav li a').click(function () {
    $('.content').hide();
    $('#' + $(this).data('content')).fadeIn(1000);
});



function fadeInFirstContent() {
    $('#content-wrap1').fadeIn(1000);

}
fadeInFirstContent();

or without adding classes to content..

 $(function(){
   $('ul#mainNav li a').click(function () {
     $('div[id^="content-wrap"]').hide();
     $('#' + $(this).data('content')).fadeIn(1000);
  });
   $('#content-wrap1').fadeIn(1000);
});

fiddle here

share|improve this answer

I think this should do the trick

for (var j = 1; j <= 5; j++) {  
    (function(i){
        $("#navItem" + i).on('click', function(){
            $('#content-wrap1').hide();
            $('#content-wrap2').hide();
            $('#content-wrap3').hide();
            $('#content-wrap4').hide();
            $('#content-wrap5').hide();
            $('#content-wrap' + i).fadeIn(1000);
        });
    })(j);
}
share|improve this answer
    
Hi, could you explain what is happening in that for loop? –  crm May 16 '13 at 12:37
    
@crm: inside the loop I'm defining a function that gets a number as parameter. Inmediatly after defining the function, I'm calling it trough this statement (j). This is a closure. –  Claudio Redi May 16 '13 at 12:41
    
How does i hold the value? Shouldn't you be calling j within the loop? also why is j in the parenthesis at the end like that? –  crm May 16 '13 at 12:44
2  
@crm: it's the magic of closures :) Here you have a SO popular question that may clarify this topic stackoverflow.com/questions/111102/…. j in the parenthesis at the end is the way to call the function I just defined with parameter j –  Claudio Redi May 16 '13 at 12:54

You don't have to change the HTML markup at all. You can make use of the starts-with CSS selector (^=):

// All elements with an ID starting with navItem...
$('[id^="navItem"]').on('click', function() {
    var
        // Get the ID as a string
        id = this.id,
        // Get the last character from the ID
        num = id.charAt(id.length-1)
    ;
    // Hide all elements with an ID starting with content-wrap...
    $('[id^="content-wrap"]').hide();
    // Fade in the relevant ID
    $('#content-wrap'+num).fadeIn(1000);
});

If we assume "navItem5" is used, here is what the variables would contain:

id == "navItem5"
num == "5"
$('#content-wrap'+num) == "#content-wrap5"

JSFiddle example.

This is very basic jQuery. You'd benefit from going through the official jQuery interactive tutorial: http://try.jquery.com.

share|improve this answer
2  
Why not just do: var id = this.id;? No need for $(this).attr('id')... It's also massively faster. –  RobH May 16 '13 at 12:59
2  
That would be a simple oversight, I've modified the answer. :) –  James Donnelly May 16 '13 at 13:01
    
Possibly include .toggle() if OP is going to use the same click to show/fadeout. –  Jonny Sooter May 16 '13 at 16:15
2  
While this answer works, it's very hacky to extract numbers out of ID attributes. –  DisgruntledGoat May 16 '13 at 22:43
2  
I’ll comment on my downvote from a few hours ago: this is incredibly fragile, and wanton abuse of IDs. What if the numbers go past 9, for example? Why not just use a clean and very efficient class? Why don't you cache that collection, or put it in an array, or something nice? –  minitech May 16 '13 at 23:37

Here's one with minimal changes to the HTML (only a common container for the contents):

$( '.mainNavItem a' ).on( 'click', function() {
    $( '#content > div' ).hide()  // hide all content divs 
                                  // (immediate div children of the main container)
        .eq( $( this ).parent().index() )  // match the index of the clicked link
                                           // with the index of the corresponding content
        .fadeIn( 1000 );          // display that div
});

http://jsfiddle.net/VQtwa/2/

share|improve this answer

It could be done much shorter: http://jsfiddle.net/2ADyp/

function hideAll(){
    $('#content-wrap1').hide();
    $('#content-wrap2').hide();
$('#content-wrap3').hide();
$('#content-wrap4').hide();
$('#content-wrap5').hide();
}

function showSelected(e){
    hideAll();
    $(e.target.hash).fadeIn(100);
}

$("#mainNav").find("a").each(function(i,o){ 
    $(o).click(showSelected);
});
share|improve this answer

Assuming you're using an expressive structure such as:

<nav>
    <a href="#content-wrap1" id="navItem1" class="navItem">One</a>
    <a href="#content-wrap2" id="navItem2" class="navItem">Two</a>
    <a href="#content-wrap3" id="navItem3" class="navItem">Three</a>
</nav>
<div id="content-wrap1" class="content-wrap">...</div>
<div id="content-wrap2" class="content-wrap">...</div>
<div id="content-wrap3" class="content-wrap">...</div>

You can then toggle the display of the content-wrap items in a short, expressive manner.

$('.navItem').on('click', function (e) {
    $('.content-wrap').hide();
    $($(this).prop('hash')).fadeIn(1000);
    e.preventDefault();
});

This does suggest changing the HTML, but only in ways that make the content semantic.

share|improve this answer
    
You should make use of the data-* attribute to store data, not the href attribute (<a data-id="#content-wrap1" .../> then you can pull it using $(this).data('id'). –  James Donnelly May 16 '13 at 16:45
1  
@JamesDonnelly, the href is more appropriate if the item being clicked on is a link. The fragment identifier is designed to point the user to a particular ID in the DOM, so even if JavaScript is disabled, the same section will be highlighted. If the item is not a link, it certainly makes sense to use a [data-*] attribute, although I'd probably use data-target or data-content-element because data-id seems a little too ambiguous. –  zzzzBov May 16 '13 at 16:53
    
if JavaScript was disabled the element wouldn't be visible anyway, so it wouldn't matter in this case. –  James Donnelly May 16 '13 at 17:58
1  
@JamesDonnelly, "the element wouldn't be visible anyway" no, if you set up your styles and scripts correctly, all the elements will be shown with javascript disabled. While it may not be what OP has set up, you have to remember that this is Code Review. –  zzzzBov May 16 '13 at 18:15
1  
Thank you. This is the perfectly right way. –  minitech May 16 '13 at 23:38

why not skip the javascript completely?

http://jsfiddle.net/zCd9g/

EDIT: sorry that's completely unhelpful seeing as this is a javascript question. Here's a solution with some modifications to the HTML:

http://jsfiddle.net/aECMb/

$('.link').click(function(e) {
    e.preventDefault();

    var $con = $('#main-content'),
        tar = $(this).data('show');

    $con.find('.contents').fadeOut();
    $con.find(tar).fadeIn();

});
share|improve this answer
    
It is ok to post a working demo on JSFiddle. Nevertheless, add the relevant code to your answer. –  nibra May 18 '13 at 10:44

I've noted some details, let me explain:


  1. CSS Selectors. jQuery allows you to use them too:

    $("[id^='content-wrap']") --> will select all elements which id starts with 'content-wrap'


  1. Substring is an available method of JavaScript:
var number = "navItem15".substring(7);
// will return what is after position 7.. 
// resulting the number "15"

  1. To start your menu with the first position. This is the best way:
(function(){
    $("#navItem1").trigger("click");
})();
// using this approach, you won't need copy your code into a function as you did
// this approach triggers the "click" event in the first menu automatically

Well, hope you got it!

Nice coding!

share|improve this answer

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.