current community

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
1

So I have the task to create a function that adds some messages to the page and another to show them in last to first order with a jQuery slideDown() effect.

So i came up with: (jsfiddle)

jQuery("#input1").click(function()
{
    //Represents the first function to add the messages
    jQuery.each(["<div>1</div>", "<div>2</div>", "<div>3</div>"],
        function(index, item)
        {
            var jDiv =jQuery(item);
            jDiv.hide();
            jQuery(".parent").prepend(jDiv);
        });

    //represents the second function to show them.
    var jParent = jQuery(".parent");
    jParent.children().reverse().each(function()
    {
        var jThis= jQuery(this);
        jParent.queue( function()
       {
           jThis.slideDown(function()
           {
               jParent.dequeue();
           });

       });
    }); 
});

(reverse from this answer)

jQuery.fn.reverse = [].reverse;

This seems to be an awful bunch of code just to show them one after another. Anyway to clean up/remove redundant code?

share|improve this question
add comment

2 Answers

up vote 1 down vote accepted

I would use function recursion in place of a jQuery queue.

The central piece is the displayNextHiddenMessagesIn function. As the name implies, it grabs the next hidden message and displays it. After the message is displayed, it calls this function again... looping until all messages are visible.

To me, this seems much more readable and easier to maintain. Also, it would be trivial to have a another message parent and reuse this same code (i.e. error messages and information messages).

(function($) {

    function createMessageFor(parent, contents) {
        var $parent = $(parent);

        for(var content in contents) {
            var $message = newMessage(contents[content]);

            $parent.prepend($message);
        }

        function newMessage(text) {
            return $("<div />").text(text).hide();
        };
    }

    var displayNextHiddenMessagesIn = function(parent) {
        var $parent = $(parent);

        $parent.find('div:hidden:last').slideDown(function() {
            displayNextHiddenMessagesIn($parent);                             
        });
    };

    $("#input1").click(function()
    {
        createMessageFor('.parent', [1,2,3]);
        displayNextHiddenMessagesIn('.parent');
    });

})(jQuery);

see it in action: http://jsfiddle.net/natedavisolds/TP66b/6/

Update

Here's what it looks like as a plugin:

(function($) {
    $.fn.fanDownHiddens = function() {
        return this.each(function() {            
            var $parent = $(this);

            $parent.find('div:hidden:last').slideDown(function() {
                $parent.fanDownHiddens();                             
            });
        });
    }            
})(jQuery);

And then the onload with the test data.

(function($) {

    $(function() {
        $("#input1").click(function() {
           setupTestMessages('.parent');
           $('.parent').fanDownHiddens();
        });
    });

     function setupTestMessages(parent) {
        var $parent = $(parent),
            testData = [1,2,3];

        for(var content in testData) {
            var $message = newMessage(testData[content]);

            $parent.prepend($message);
        }

        function newMessage(text) {
            return $("<div />").text(text).hide();
        };
    }
})(jQuery);

Here's the plugin working: http://jsfiddle.net/natedavisolds/TP66b/12/

share|improve this answer
 
That looks interesting but I'd like to note two things : 1. has message seems unnecessary. if $nextMessage is empty nothing will happen. 2. Doesn't seem any real different to mine except that it looks for the last hidden ... or did I miss something. I like that thought though. –  James Khoury Mar 5 '12 at 1:30
 
Also the function you've named createMessageFor is just a stub for another unrelated ajax method. I only added it to the question so people could see what I was getting. –  James Khoury Mar 5 '12 at 1:32
 
@JamesKhoury yep.. you are absolutely right. I missed that. I've corrected it above. The find is the main difference and not using queue. I wonder how this would look as a plugin. –  natedavisolds Mar 5 '12 at 3:00
 
@JamesKhoury I also added the plugin for review. –  natedavisolds Mar 5 '12 at 4:06
add comment
up vote 1 down vote

I skipped the second button for simplicity's sake.

http://jsfiddle.net/WjaDE/7/

var jParent = $("#parent"); 
// used an id for the unique element
// cached the element up front
var DURATION = 500;

$("#input1").click(function() {
    $.each(['1', '2', '3'], // simplified the array
        function(index, item) {
            var jDiv = $('<div class=box></div>');
            // added a class to each to improve the css and eliminated the .hide()
            jDiv.html( item );
            jParent.prepend(jDiv);
            jDiv.delay(DURATION * index);
            // used a variable delay to deal with the timing
            jDiv.slideDown(DURATION);
        });
});
share|improve this answer
 
The second button was from the jQuery site. I just had it there for testing earlier. The code was in separate sections as they are in different functions. They can't be combined together. But i do like the idea of a Delay. –  James Khoury May 8 '11 at 5:17
add comment

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.