How do I write this jQuery function so that's it's clean and efficient?

Question

I have written some jQuery (with much help from Stack Overflow!) that slides open a panel on click, and changes the toggles of the clickable bit from 'Show All' to 'Hide All'.

It works, but I'm trying to figure out what would be the most efficient way to do it, as it seems kind of long to me, and some HTML is repeated.

I've tried using variables and assigning the HTML to be repeated, but it doesn't work and I can't figure out where the problem in my logic is!

This is the working version and a working fiddle here :

  $(function () {
    //Hide panel with jQuery and create toggle button so subjects will just show up if there's no javascript
    $("#panelSub").hide();
    $('#toggleSubjects').html('<h4 class="subjectOpen"><i class="fa fa-arrow-circle-down"></i>Show All Subjects<i class="fa fa-arrow-circle-down"></i></h4>');

    //Toggle extra subject panel
    $("#toggleSubjects").click(function () {
        $("#panelSub").slideToggle();
        //Change toggle text for show / hide
        if ($.trim($('.subjectOpen').text()) === 'Show All Subjects') {
            $(this).html('<h4 class="subjectOpen"><i class="fa fa-arrow-circle-up"></i>Hide All Subjects<i class="fa fa-arrow-circle-up"></i></h4>');
        } else {
            $(this).html('<h4 class="subjectOpen"><i class="fa fa-arrow-circle-down"></i>Show All Subjects<i class="fa fa-arrow-circle-down"></i></h4>');
        }
    });
    return false;
});

And this is the 'more efficient' non-working version I've been trying, apologies in advance for how bad it is :/

    var pOpen = '<h5 class="subjectOpen"><i class="fa fa-arrow-circle-down"></i>Show All Subjects <i class="fa fa-arrow-circle-down"></i></h5>';
var pClose = '<h5 class="subjectOpen"><i class="fa fa-arrow-circle-down"></i>Show All Subjects <i class="fa fa-arrow-circle-down"></i></h5>';
var p = $('#toggleSubjects').html();

$(function() {
//Hide panel with jQuery and create toggle button so subjects will just show if there's no javascript
$("#panelSub").hide();

var p = pOpen.html().appendTo('panelSub');
//Toggle subject panel
$("#toggleSubjects").click(function(){
    $("#panelSub").slideToggle("slow");
    //Change toggle text for show / hide
        if ($.trim($('.subjectOpen').text()) === 'Show All Subjects') {
        $(this).html() === pClose.html();
        } else {
        $(this).html()=== pOpen.html();    
            }
        });
        return false;
 });

Dai this is too difficult to do da.. And here in SO code shrinking works will not be done.. You can post your question here http://codereview.stackexchange.com/ — Rajaprabhu Aravindasamy, Apr 22 '15 at 09:13
This question belongs on another site in the Stack Exchange network: http://codereview.stackexchange.com/ — Satpal, Apr 22 '15 at 09:14
This should be on the Codereview site, but I would turn that IF into a Turnary if and i'd turn that $('.subjectOpen') into a var outside of the function. otherwise everytime it runs the click function it needs to search the DOM for that element. Thats more for performance than physical code size. — Josh Stevenson, Apr 22 '15 at 09:15

score 0 · Answer 1 · answered Apr 22 '15 at 09:24

0

i dont understand this line;

var p = pOpen.html().appendTo('panelSub');

But change this lines;

$(this).html() === pClose.html();
$(this).html()=== pOpen.html();

to:

$(this).html(pClose);
$(this).html(pOpen);

answered Apr 22 '15 at 09:24

Eren Akkus

471
4
11

gabitzish · Accepted Answer · 2015-04-22T11:06:40.900

0

Please check out this updated jsFiddle: https://jsfiddle.net/emerfan/2oqxL8hx/2/

I've changed your JS code into :

$(document).ready(function () {
    var theToggle = '<div class="panel-heading" id="toggleSubjects"><h5 class="subjectOpen"> <i class="fa fa-arrow-circle-down"></i> <i class="fa fa-arrow-circle-down"></i></h5></div>';
    $("#panelSub").hide();
    $(".panel-default").append(theToggle);
    $("#toggleSubjects").click(function () {
        $("#panelSub").slideToggle("slow");                
        $(this).find('i').toggleClass('fa-arrow-circle-down fa-arrow-circle-up')
    });
    return false;
});

and updated your HTML and CSS to hold all your structure and content. It's a good practice to keep things separated (HTML, CSS and JS)

Later edit: I've updated with the code that fits your needs.

edited Apr 22 '15 at 11:06

answered Apr 22 '15 at 09:32

gabitzish

9,535
7
44
65

Hey that's great thanks! except for one thing! If there is no JS, the 'Show All Subjects' 'Hide All Subjects bar shoudn't appear, so it needs to be created with the function :) – Apr 22 '15 at 09:48
Also, would you mind explaining this useful looking line? $(this).find('i').toggleClass('fa-arrow-circle-down fa-arrow-circle-up')... I think it means, find the first instance of the fa-arrow classes and toggle it? – Apr 22 '15 at 09:51
Here's an explanation for that: http://stackoverflow.com/questions/7002039/easiest-way-to-toggle-2-classes-in-jquery – gabitzish Apr 22 '15 at 09:59
I have edited your code a little bit and created the toggle with jquery, fiddle is here https://jsfiddle.net/emerfan/2oqxL8hx/2/ , if you can update your answer I'll mark it as the answer, thanks a lot ! :) – Apr 22 '15 at 10:21

How do I write this jQuery function so that's it's clean and efficient?

2 Answers2