4
\$\begingroup\$

Here's a fiddle: http://jsfiddle.net/YF8cg/

Focussing entirely on the javascript and on the structure of the HTML (.qapair .question and .answer), how could I improve this system?

JS:

$(document).ready(function() {
    $('.question').click(function() {
        $(this).siblings('.answer').slideToggle(400);       // Display the clicked answer

        $(this).parents('.QApair').siblings('.QApair').children('.answer').each(    // Hide all the others
            function() {
                if($(this).is(':visible')) {    // Detects if it's visible
                    $(this).slideToggle(400);   // If true then toggles it
                }
        });
    });
});

HTML:

<div class="qapair">
        <h4 class="question">First Question</h4>
        <div class="answer">Here would be lods of boring text, or some images, or some tables, or some fish, or some lets-pick-another-random-nouns, you get the picture. I just needed to fill space.
        </div>
    </div>
    <div class="qapair">
        <h4 class="question">Second Question</h4>
        <div class="answer">I've run out of anything to write. So keyboard spas in 5... 4... 3... 2... 1... agdkfdakgjajgjregafgi iej fujagijrfgj rhgjahngufg ughuafng  uuagj u unhu up  u dnfajng hfgnajgnurngr gugnjfngja uugf n ndnfaung urgjangjnfg. That was interesting....
        </div>
    </div>

I'm fairly new to jQuery, so any help would be appreciated.

Thanks in advance.

\$\endgroup\$
  • 1
    \$\begingroup\$ Cache your selectors for starters \$\endgroup\$ – Benjamin Gruenbaum Apr 11 '13 at 9:50
2
\$\begingroup\$

HTML: The only change is that they are wrapped in a div id'ed container which will be used in event handling, explained below.

<div id="container">
    <div class="qapair">
         <h4 class="question">First Question</h4>
         <div class="answer"></div>
    </div>
    <div class="qapair">
         <h4 class="question">Second Question</h4>
         <div class="answer"></div>
    </div>
    <div class="qapair">
         <h4 class="question">Third Question</h4>
         <div class="answer"></div>
    </div>
</div>

JS: Explained in the comments

//shorthand for $(document).ready(fn) is $(fn)
$(function () {

        //container will be used for delegated event handling
    var container = $('#container'),
        //since questions remain static, it's best we reference ahead of time
        questions = $('.question',container);

    //take advantage of delegation which assigns one handler to the parent
    //for all events in it's descendant. Here, we place a handler on container
    //for all question instead of putting a handler per question
    container.on('click', '.question', function (event) {
        var question = $(this);

        //we use 'next' which directly gets the next sibling instead of siblings(selector). 
        //though internal implementations might be the same, but at least we avoid
        //writing that many selectors in our implementation
        question
            .next()
            .slideToggle(400);

        //using the referenced questions, we remove the currently clicked, pick
        //their answers that are currently visible and slide them up
        questions
            .not(question)
            .next(':visible')
            .slideUp(400);
    });
});

Here's running code. It's more of a readability and maintainability optimization rather than performance.

\$\endgroup\$
  • \$\begingroup\$ Thanks. I learnt some intersting things from that, notable the delegation part. I'd upvote if I had enough rep, but for the time being I've just selected as answer. \$\endgroup\$ – Luke Ashford Apr 11 '13 at 10:25

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.