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>
asked Mar 21, 2013 at 15:46
\$\endgroup\$
3
  • 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\$ Commented Mar 21, 2013 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\$ Commented Mar 21, 2013 at 18:21
  • \$\begingroup\$ @Yuck: Obviously it was a well-intentioned edit but Loki's right, and here are additional reasons why. \$\endgroup\$ Commented Mar 21, 2013 at 19:52

2 Answers 2

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');
 }
}
answered Mar 22, 2013 at 2:05
\$\endgroup\$
2
  • \$\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\$ Commented Mar 22, 2013 at 8:44
  • \$\begingroup\$ I can't upvote yet as I'm new, thanks again though. \$\endgroup\$ Commented Mar 22, 2013 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');
 });
});
answered Mar 21, 2013 at 23:09
\$\endgroup\$
4
  • 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\$ Commented Mar 21, 2013 at 23:58
  • \$\begingroup\$ @Tyriar To maintain similarity. \$\endgroup\$ Commented Mar 21, 2013 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\$ Commented Mar 22, 2013 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\$ Commented Mar 22, 2013 at 0:08

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.