4
\$\begingroup\$

I'm new to jQuery and I've been messing about with this code, It works but I want to learn how to shorten the code by the eliminating unnecessary repeated code.

Here is a link to JSFiddle

HTML

<div id="div1" style="display:none;">one<div id="divb" class="upper">close</div></div>
<div id="div2" style="display:none;">two<div id="divb" class="upper">close</div></div>
<div id="div3" style="display:none;">three<div id="divb" class="upper">close</div></div>
<div id="div4" style="display:none;">four<div id="divb"     class="upper">close</div></div>
</br></br></br></br></br></br></br></br></br></br></br><div class="buttons">
<a class="button" id="showdiv1">Div 1</a>
<a class="button" id="showdiv2">Div 2</a>
<a class="button" id="showdiv3">Div 3</a>
<a class="button" id="showdiv4">Div 4</a>
</div>

JQuery

$(function() {
     $('#showdiv1').click(function() {
        $('html, body').animate({ scrollTop: 0 }, 'slow');
         $('div[id^=div]').slideUp().delay(1000);
          $('#div1').slideDown('slow').delay(1000);

    });

      $('.upper').click(function() {
      $('html, body').animate({ scrollTop: 0 }, 'slow');
       $('#div1').slideUp('slow');
    });

    $('#showdiv2').click(function() {
        $('html, body').animate({ scrollTop: 0 }, 'slow');
         $('div[id^=div]').slideUp().delay(1000);
          $('#div2').slideDown('slow').delay(1000);

    });

     $('.upper').click(function() {
      $('html, body').animate({ scrollTop: 0 }, 'slow');
       $('#div2').slideUp('slow');
    });


     $('#showdiv3').click(function() {
        $('html, body').animate({ scrollTop: 0 }, 'slow');
         $('div[id^=div]').slideUp().delay(1000);
          $('#div3').slideDown('slow').delay(1000);

    });

     $('.upper').click(function() {
      $('html, body').animate({ scrollTop: 0 }, 'slow');
       $('#div3').slideUp('slow');
    });

   $('#showdiv4').click(function() {
        $('html, body').animate({ scrollTop: 0 }, 'slow');
         $('div[id^=div]').slideUp().delay(1000);
          $('#div4').slideDown('slow').delay(1000);

    });

     $('.upper').click(function() {
      $('html, body').animate({ scrollTop: 0 }, 'slow');
       $('#div4').slideUp('slow');
    });

})

Ok, after much headache - I'm new to this sh#t. I've changed the code to this. All works fine. I would like someone that knows, to give me any tips for improvement, as I'm totally improvising here, thankyou.

jquery

$(document).ready(function(){

    $('a').click(function () {
    var divname= this.name;
    $('html, body').animate({ scrollTop: 0 }, 'slow');
    $('div[id^=div]').slideUp().delay(1000);
      $("#"+divname).slideDown('slow').delay(1000);

    });

     $('.upper').click(function() {
    $('html, body').animate({ scrollTop: 0 }, 'slow');
    $('div[id^=div]').slideUp('slow');

  });
    });

More HTML

<body>
<div id="div1" style="display:none">
Hello World  <a class="upper" id="div1">close</a>
</div>
<div id="div2" style="display:none">
Test<a class="upper" id="div2">close</a>
</div>
<div id="div3" style="display:none">
Another Test
</div>
<div id="div4" style="display:none">
Final Test
</div>
</br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br>
<div class="menu">
<ul>
<li><a href="#" name="div1" >one</a></li>
<li><a href="#" name="div2" >two</a></li>
<li><a href="#" name="div3" >three</a></li>
<li><a href="#" name="div4" >four</a></li>
</ul>
</div>
</body>
\$\endgroup\$
  • 4
    \$\begingroup\$ Hi and welcome to Code Review. Please post your code in the question as well. Also make sure to check this out: codereview.stackexchange.com/faq \$\endgroup\$ – Jonny Sooter Mar 21 '13 at 17:06
  • \$\begingroup\$ @Yuck: You can't post people code for them. As this page gives away rights via collective comments. You have to ask them to post their own code. \$\endgroup\$ – Martin York Mar 21 '13 at 18:21
  • \$\begingroup\$ @Yuck: Obviously it was a well-intentioned edit but Loki's right, and here are additional reasons why. \$\endgroup\$ – seand Mar 21 '13 at 19:52
2
\$\begingroup\$

View all my changes on this jsFiddle.

HTML

  • Use the self-closing tag <br /> not the closing tag </br>.
  • Your close buttons (.upper) are doing a very similar function to your <a> tags below, the should both be <a> tags. The reason why I wouldn't make them <div>s is because you would need to manually add a tab-index to enable keyboard accessibility, links come with it.
  • I'd use a HTML5 data- attribute instead of ID to specify the id of the div you're showing.
  • No need to include an id on the close buttons.
  • You can add a class to your 'sections' so they can be targeted together.

    <div id="div1" class="section">
        one
        <a class="hide-div">close</a>
    </div>
    <div id="div2" class="section">
        two
        <a class="hide-div">close</a>
    </div>
    <div id="div3" class="section">
        three
        <a class="hide-div" href="#">close</a>
    </div>
    <div id="div4" class="section">
        four
        <a class="hide-div" href="#">close</a>
    </div>
    
    <br /><br /><br /><br /><br /><br /><br /><br /><br />
    
    <div class="buttons">
        <a class="show-div" data-div="div1" href="#">Div 1</a>
        <a class="show-div" data-div="div2" href="#">Div 2</a>
        <a class="show-div" data-div="div3" href="#">Div 3</a>
        <a class="show-div" data-div="div4" href="#">Div 4</a>
    </div>
    

CSS

  • Put the display:none; style into your CSS.

    .section {
        height:600px;
        display:none;
    }
    
    .hide-div {
        display:block;
    }
    

JavaScript

  • Target the .button class, not a. I would also rename .button to something a little more descriptive like .show-div
  • .upper isn't very descriptive, something like .hide-div would be better.
  • You should only hide and delay the sections if there are any, this gives the user a weird second long pause if there are no sections visible. I've used the .visible class to aid with this.
  • Pulled the hide sections code out because it's used in multiple places.

    $(document).ready(function () {
        $('.show-div').click(function () {
            var divId = this.getAttribute('data-div');
            $('html, body').animate({
                scrollTop: 0
            }, 'slow');
    
            hideSections();
    
            $("#" + divId)
                .slideDown('slow')
                .delay(1000)
                .addClass('visible');
        });
    
        $('.hide-div').click(function () {
            $('html, body').animate({
                scrollTop: 0
            }, 'slow');
            hideSections();
        });
    });
    
    function hideSections() {
        if ($('.visible').length > 0) {
            $('.section')
                .slideUp()
                .delay(1000)
                .removeClass('visible');
        }
    }
    
\$\endgroup\$
  • \$\begingroup\$ Thank you Tyriar and Back in a Flash, very helpful. Lots of great tips, I'm very pleased. May the learning curve continue. \$\endgroup\$ – Rico Shaft Mar 22 '13 at 8:44
  • \$\begingroup\$ I can't upvote yet as I'm new, thanks again though. \$\endgroup\$ – Rico Shaft Mar 22 '13 at 8:54
3
\$\begingroup\$

Your newer code is much much better than the previous attempt. Some little fixes that I'd suggest are:

  1. Don't use the following

    $('div[id^=div]').slideUp().delay(1000);
    

    as it hides the close button inside the div. So, if I open div1 and click the div1 again, I can't see the close button there.

  2. Since you are continually using jQuery, I'd prefer to use .attr() method to fetch the name instead of

    this.name;
    

    that you have used inside your new code.

  3. You must not bind your .click() to the a tag. Instead you already have class="button" assigned to them. Use it.

Here is an updated fiddle link that addresses the issues and fixes them.

jQuery Code

$(function () {
    var newId = "";
    $('.button').click(function () {
        $('html, body').animate({
            scrollTop: 0
        }, 'slow');
        if (newId !== "") $('#' + newId).slideUp().delay(1000);
        newId = $(this).attr("name");
        $('#' + newId).slideDown('slow').delay(1000);

    });
    $('.upper').click(function () {
        $('html, body').animate({
            scrollTop: 0
        }, 'slow');
        $(this).parent('div').slideUp('slow');
    });
});
\$\endgroup\$
  • 1
    \$\begingroup\$ I strongly disagree with number 2. Even though he is using jQuery, jQuery is a framework on top of JavaScript. Why take the effort to create a jQuery object and then call a far slower method to produce the EXACT same result when this.name is faster and more accessible. \$\endgroup\$ – Daniel Imms Mar 21 '13 at 23:58
  • \$\begingroup\$ @Tyriar To maintain similarity. \$\endgroup\$ – hjpotter92 Mar 21 '13 at 23:59
  • \$\begingroup\$ The language being used is JavaScript though. I understand the importance of code consistency, but there is nothing wrong with using simple object properties when you're using jQuery. \$\endgroup\$ – Daniel Imms Mar 22 '13 at 0:07
  • 1
    \$\begingroup\$ Yup, @Tyriar which is why I mentioned that I'd prefer to use as opposed to you shouldn't. \$\endgroup\$ – hjpotter92 Mar 22 '13 at 0:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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