current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

I have here some code which is working but it seems to be a lot of coding to do here. I would really appreciate if someone could make this simpler.

<!-- *********************************************** -->
<div class="widget search-w">
    <div class="wrapper">
        <div class="big-icon search icontap"><a class="widget-close" data-toggle="collapse" data-target="#search">click</a>

        </div>
        <div class="s-title"><a class="widget-close closeme">Close</a></div>

        <div id="search" class="collapse in biz-widget">
            <div>                    
                <?php get_search_form(); ?>

                <div class="widget-content">
                    <h4>Your recent searches</h4>
                    <ul>
                        <li><a href="">search 1</a></li>
                        <li><a href="">search 1</a></li>
                        <li><a href="">search 1</a></li>
                        <li><a href="">search 1</a></li>
                        <li><a href="">search 1</a></li>
                    </ul>
                </div>

            </div>
        </div>
    </div>
</div>

<!-- *********************************************** -->
<div class="widget favourite-w">
    <div class="big-icon favourite icontap2"><a class="widget-close" data-toggle="collapse" data-target="#favourite">click</a></div>
    <div class="s-title"><a class="widget-close closeme2">Close</a></div>

    <div id="favourite" class="collapse in biz-widget">

        <ul>
            <li><a href="">favourite page</a></li>
            <li><a href="">favourite page</a></li>
            <li><a href="">favourite page</a></li>
            <li><a href="">favourite page</a></li>
            <li><a href="">favourite page</a></li>
        </ul>

    <div class="clearfix"></div>
    </div> <!-- end #about -->
    <div class="clearfix"></div>
</div>

and here is my js

$(".icontap").toggle(function(){
    $("#search").animate({height:40},200).css( "overflow", "hidden" );
    $(".search-w .widget-close").css("visibility","visible");
    $(".search-w .biz-widget").css("border-color", "#335ccc");
},function(){
    $("#search").animate({height: $("#search").get(0).scrollHeight}, 200 );
    $(".search-w .widget-close").css("visibility","visible");
    $(".search-w .biz-widget").css("border-color", "#335ccc");
});

$(".closeme").click(function(){
    $("#search").animate({height:0},200);
    $(".search-w .biz-widget").css("border-color", "#fff");
    $(".search-w .widget-close").css("visibility","hidden");
});

/*******/

$(".icontap2").toggle(function(){
    $("#favourite").animate({height:40},200).css( "overflow", "hidden" );
    $(".favourite-w .widget-close").css("visibility","visible");
    $(".favourite-w .biz-widget").css("border-color", "#335ccc");
},function(){
    $("#favourite").animate({height: $("#favourite").get(0).scrollHeight}, 200 );
    $(".favourite-w .widget-close").css("visibility","visible");
    $(".favourite-w .biz-widget").css("border-color", "#335ccc");
});

$(".closeme2").click(function(){
    $("#favourite").animate({height:0},200);
    $(".favourite-w .biz-widget").css("border-color", "#fff");
    $(".favourite-w .widget-close").css("visibility","hidden");
});

Or see this in fiddle.

share|improve this question
    
Welcome to CodeReview, Oliver. I hope you get some fine answers. –  Legato Apr 8 at 23:00

2 Answers 2

up vote 1 down vote

The way I see it there are two accordion styles.

  1. Expand and Contract (ec) the one Section where multiple Sections can be Expanded or Contracted.
  2. Only one Section is Expanded at any given time. (ecp ec paired)

HTML

<button class="link" type="button" onclick ="ec('s1')">Button</button>
<div id="s1" class="hide">

</div>

<button class="link" type="button" onclick ="ec('s2')">Button</button>
<div id="s2" class="hide">

</div>
<div id="s999"></div>

JS

I have two functions one for each the two EC methods.
function ec() is for EC method 1
functions ecp() is for EC method 2, pairs the current selected section with the previous.
Normally obe one of these functions is used on a web page.

The div id="s999" is for the initial value for prev so there is no null value to prev.

The reason I have var disp = div.style.display; is for when the same section is selected as previously. I do not care if prev is expanded or contracted just contracting it is much quicker than an else if to check its display state. But I need to remember what I am doing with the passed id.

If the same section is clicked and I toggle the display state, if it is being expanded the prev toggle would contract it again.

The line of code:

var y=div.offsetTop;window.scrollTo(0, y-64);

Takes the expanded section and puts it at the top of the Browser window. When all are contracted, it scrolls to the very top or the page.

The reason I use an array for the toggle is to eliminate the else if that would otherwise be required.


Working Snippet

  var toggle=[]
 toggle[''] = 'block';
    toggle['none'] = 'block';
    toggle['block'] = 'none';
    function ec(id){
      div = document.getElementById(id);
      div.style.display=toggle[div.style.display];
      var y=div.offsetTop;window.scrollTo(0, y-24);
    }



    var prev = document.getElementById('s999');
    function ecp(id){ 
      div = document.getElementById(id);
      var disp = div.style.display;
      prev.style.display='none';
      div.style.display=toggle[disp];
      prev=div;
      var y=div.offsetTop;window.scrollTo(0, y-24);
    }
body{background:#333;font-size:.8em;}
.btn1,.btn2{padding: .2em;margin:.2em 0 0 0;border-radius: 3px 3px 3px 3px;font: 700 1em Arial,Helvetica,Calibri,sans-serif;overflow: visible;border:1px solid #2985EA;color: #fff;background-color:#2985EA;
background-image: -o-linear-gradient(bottom, #2ef 0%, #02f 100%);
background-image: -moz-linear-gradient(bottom, #2ef 0%, #02f 100%);
background-image: -webkit-linear-gradient(bottom, #2ef 0%, #02f 100%);
background-image: -ms-linear-gradient(bottom, #2ef 0%, #02f 100%);
background-image: linear-gradient(to bottom, #2ef 0%, #02f 100%);}
.btn1{width:96%;}
.btn2{width:48%;}
h3{margin:0;}
p{margin:.1em .4em;}
.s1,.s2,.s3,.s4{display:none;padding:0;margin:0;}
.s1{color:#f0f;}
.s2{color:#0f0;}
.s3{color:#f00;}
.s4{color:#0ff;}
#s999{min-height:40em;}
<!DOCTYPE html>
<html lang="en"><head><title>Expand Contract</title></head><body>
<button class="btn1" type="button" onclick ="ec('s1')">Section One </button><br/>
<div id="s1" class="s1">
<h3>This Section toggles when Section One button is clicked</h3>
<p>Section 1 content<br/>goes here.</p>
</div>
<button class="btn1" type="button" onclick ="ec('s2')">Section Two</button><br/>
<div id="s2" class="s2">
  <h3>This Section toggles when Section Two button is clicked</h3>
<p>Section 2 content<br/>goes here.</p>
</div>
<button class="btn2" type="button" onclick ="ecp('s3')">Section Three </button>
<button class="btn2" type="button" onclick ="ecp('s4')">Section Four</button>

<div id="s3" class="s3">
<h3>This Section toggles when Section Three button is clicked<br>And if Expanded will Contract when Button Four is clicked</h3>
<p>Section 3 content<br/>goes here.</p>
</div>
 <div id="s4" class="s4">
  <h3>This Section toggles when Section Four button is clicked<br>And if Expanded will Contract when Button Three is clicked</h3>
<p>Section 4 content<br/>goes here.</p>
</div>
<div id="s999"></div> 

<div style="height:40em"><p>&#x2002;</p></div>      
  </body></html>

share|improve this answer
up vote 1 down vote

There's a best practice here that I'd like to describe which will actually provide a range of benefits to your app, in addition to the shortening of code as you've asked.

The answer is to move changes of style, in response to click/toggle events, from the JavaScript and into the CSS.

For example, taking the '.closeme2' click handler at the bottom as an example...

Before:

$(".closeme2").click(function() {
    $("#favourite").animate({height:0},200);
    $(".favourite-w .biz-widget").css("border-color", "#fff");
    $(".favourite-w .widget-close").css("visibility","hidden");
});

After:

// JS
$(".closeme2").click(function() {
    $('.favourite-w').addClass('is-closed');
});

// CSS
.favourite-w.is-closed {
    height: 0;
}
.favourite-w.is-closed .biz-widget {
    border-color: #fff;
}
.favourite-w.is-closed .widget-close {
    visibility: hidden;
}

Reasons why:

  • Separation of concerns, your styling belongs in your CSS files rather than JavaScript (i.e. component styles all together)
  • Reusable code, these styles reflecting the "closed" state could be re-used across all instances of this UI component, as well as others components, rather than re-written every time in JS click handler functions
  • Modifying CSS via jQuery functions such as 'css(property, value)' is a bad idea, it results in the styles added as inline styles in the DOM (e.g. style="border-color: #fff;") and this will take priority over other CSS, making managing your CSS harder and debugging presentation errors more difficult

If you apply the above to all of your click handlers and UI components, you will both reduce the length of your JavaScript code, and clarify it, significantly (as well as implement a number of general best practices for wider benefits).

Height Animation

The above code will not provide the height animation that you currently have.

The solution here is to adopt a CSS transition approach rather than animating via JavaScript. Article: http://www.w3schools.com/css/css3_transitions.asp

This will again mean that all of your component related styling will remain together in one place, but CSS animation also performs much better/renders faster than JavaScript animation does.

Hope this helps.

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.