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?

  • 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
up vote 31 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);
});
  • 1
    This is much better than using a substring of the ID. Also, you’re missing a . in front of navItem – Ry- 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

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.

  • 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? – Ry- May 16 '13 at 23:37

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);
}
  • 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

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

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);
});

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.

  • 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

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/

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();

});
  • 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!

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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