HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Simple element slider animation tool

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simpleanimationsliderelementtool

Problem

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

oneclose
twoclose
threeclose
fourclose

Div 1
Div 2
Div 3
Div 4


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').cli

Solution

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

-
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.

-
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.

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

Code Snippets

$('div[id^=div]').slideUp().delay(1000);
$(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');
    });
});

Context

StackExchange Code Review Q#24207, answer score: 3

Revisions (0)

No revisions yet.