Clean up jQuery script

Question

I combined multiple jQuery scripts into one script. This script converts a <ul> element to a <select> box (for use in Response WebDesign). But I think this script is not well written, can someone help me clean up this script?

(function($) {
    $(function() {
        var $select = $('<select>').appendTo('#nav');
        $('#nav li').each(function() {
            var $li = $(this),
                $a = $li.find('> a'),
                $p = $li.parents('li'),
                prefix = new Array($p.length + 1).join('-');
            var $option = $('<option>')
                .text(prefix + ' ' + $a.text())
                .val($a.attr('href'))
                .appendTo($select);
        });
        $(function() {
            $("<option>", {
                "selected": "selected",
                "value": "",
                "text": "Navigate to..."
            }).prependTo("#nav select");
        });
        $(function() {
            $('#nav select').bind('change', function() {
                var url = $(this).val();
                if (url) {
                    window.location = url;
                }
                return false;
            });
        });
    });
})(jQuery);

Update

Thanks John, I have re-written the script tested only he does not word. Only when I use the function (function ($) { append to each function

Like this:

(function($) {
    $(function() {
        var select = $('<select>').appendTo('#nav');
        $('#nav li').each(function() {
            var li = $(this),
                a = li.find('> a'),
                p = li.parents('li'),
                prefix = new Array (p.length + 1).join('-');
            var option = $('<option>')
                .text(prefix + ' ' + a.text())
                .val(a.attr('href'))
                .appendTo(select);
        });
    });
    $(function() {
        $("<option>", {
            "selected": "selected",
            "value": "",
            "text": "Navigate to..."
        }).prependTo("#nav select");
    });
    $(function() {
        $('#nav select').on('change', function() {
            var url = $(this).val();
            if (url) {
                window.location = url;
            }
            return false;
        });
    });
})(jQuery);

Update 2:

Is this one more like it?

$(function() {
    $("<select />").appendTo("#nav");
    $("<option />", {
        "selected": "selected",
        "value": "",
        "text": "Navigate to..."
    }).appendTo("#nav select");
    $("#nav li").each(function() {
        var li = $(this),
            a = li.find("> a"),
            p = li.parents("li"),
            prefix = new Array(p.length + 1).join("-");
        var option = $("<option />")
            .text(prefix + " " + a.text())
            .val(a.attr("href"))
            .appendTo("#nav select");
    });
    $("#nav select").change(function() {
        window.location = $(this).find("option:selected").val();
    });
});

Update 3:

Thnx RobH. Zo this will be the final (good) code? I dont know which is the best they work both (update 2 and 3)

jQuery(document).ready(function($){ 
    var options = [];
    $("<select />").appendTo("#nav").append($("<option />", {
        "selected": "selected",
        "value": "",
        "text": "Navigate to..."
    }));
    $("#nav li").each(function() {
        var li = $(this),
            a = li.find("> a"),
            p = li.parents("li").length,
            arr = [],
            prefix = '';
        arr[p] = '';
        prefix = arr.join('-');
        options.push($("<option />")
            .text(prefix + " " + a.text())
            .val(a.attr("href")));
    });
    $("#nav select").append(options).on('change', function() {
        var url = $(this).val();
        if (url) {
            window.location = url;
        }
        return false;
    });
});

Here is the html it needs to work with (Jonny Sooter asked me to post this):

    <div id="nav">
        <ul id="menu">
            <li><a href="index.html" title="Level 1">Level 1</a></li>
            <li><a href="#" title="Section 1">Section 1</a>
                <ul>
                    <li><a href="index.html" title="Level 2">Level 2</a></li>
                    <li><a href="#" title="Section 2">Section 2</a>
                        <ul>
                            <li><a href="index.html" title="Level 3">Level 3</a></li>
                            <li><a href="index.html" title="Level 3">Level 3</a></li>
                            <li><a href="index.html" title="Level 3">Level 3</a></li>
                            <li><a href="index.html" title="Level 3">Level 3</a></li>
                        </ul>
                    </li>
                    <li><a href="index.html" title="Level 2">Level 2</a></li>
                    <li><a href="index.html" title="Level 2">Level 2</a></li>
                </ul>
            </li>
            <li><a href="index.html" title="Level 1">Level 1</a></li>
            <li><a href="index.html" title="Level 1">Level 1</a></li>
        </ul>
    </div>

Update 4

For the hide an show stuff i had a css solution

/* menu-view css */
#nav select { display: none; }
#nav ul { display: inline; }

@media screen and (max-width: 979px) {
/* menu-mobile-view css */
    #nav ul { display: none; }
    #nav select { display: inline-block; }  

}

Thanks for your comment tomdemuyt. I see that there are a lot of things are wrong. The script works well only it's cut and paste job. I also think that everything goes through each other. And I have no idea what you mean with the changes and where I should apply. Could you help me with this? So that the script is cleaner and in the future will continue to work?
Does your latest update work for you? Have you tested it with your HTML? Could you post a copy of that HTML as well? I've also included an update in my answer for your prefix part.
One last thing you might want to add is hiding the ul to leave only the select showing. Since it's responsive that's probably what you'll want to do. It can be just a simple $("#menu").hide();

tomdemuyt · Answer 1 · 2013-03-11 16:43:57Z

Some observations in random order:

You are not using $option anywhere, why declare a var?

You are using $a only for the href, why not

var href =  $li.find('> a').attr('href');

Same for var $p vs var length
It is not clear in the code where (if) you do the replacing
You do not need to repeat the last 2 $(function() {
I personally dislike prefixing vars with $

Jonny Sooter · Answer 2 · 2013-03-28 14:18:32Z

Here are some comments on your code. I tried to add new information since there has been an answer already.

(function($) {
    //We only need one IIFE that'll invoke the whole code. You shouldn't need the other functions, unless you name them and call them individualy for some reason.
    var select = $('<select>').appendTo('#nav'); //I personally don't like to use "$" in front of variables, regardles if they are a jQuery object.
    $('#nav li').each(function() {
        var li = $(this),
            a = li.find('> a'),
            p = li.parents('li'),
            //Use the literal array notation []
            //After using .join() it becomes a string, so consider making it a string to begin with instead of an array
            prefix = [p.length + 1].join('-');

        var option = $('<option>')
            .text(prefix + ' ' + a.text())
            .val(a.attr('href'))
            .appendTo(select);
    });

    $("<option>", {
        "selected": "selected",
        "value": "",
        "text": "Navigate to..."
    }).prependTo("#nav select");

    $('#nav select').on('change', function() { //We don't use .bind() as of 1.7 any more, use ".on" instead.
        var url = $(this).val();
        if (url) {
            window.location = url;
        }
        return false;
    });
})(jQuery);

UPDATE:

I just realized a big no no in your code. It's the .appendTo() in the .each() loop.

$('#nav li').each(function() {
    var li = $(this),
        a = li.find('> a'),
        p = li.parents('li'),
        prefix = [p.length + 1].join('-');

    var option = $('<option>')
        .text(prefix + ' ' + a.text())
        .val(a.attr('href'))
        .appendTo(select); //Don't do this
});

What happens when you do that is jQuery will do that append method each time it finds an element that matches. That's bad because it will keep doing that and slowing you down. It's better to go through the loop and save what you want to append to a variable. Then just append that variable once. Something like this should do the trick:

var appendOption = []; //We can define an array here if you need to play with the results in order or just use a string otherwise.

$('#nav li').each(function() {
    var li = $(this),
        a = li.find('> a'),
        p = li.parents('li'),
        prefix = [p.length + 1].join('-');

    var option = $('<option>')
        .text(prefix + ' ' + a.text())
        .val(a.attr('href'))
        .push(appendOption); //This adds each thing we want to append to the array in order.
});

//Out here we call the append once
//Since we defined our variable as an array up there we join it here into a string
appendOption = appendOption.join(" ");
select.append(appendOption);

Here's some reading materials you should check out:

http://www.learningjquery.com/2009/03/43439-reasons-to-use-append-correctly

2nd UPDATE

A fix for your prefix:

$("#nav li").each(function(i) { //We want to get the index or "i"
    var li = $(this),
        a = li.find("> a"),
        p = li.parents("li"),

        //Here's the prefix update:
        prefix = i + 1 + "-";
        //This adds a number to each one.
        //You could make it so it adds a "." between the levels as well like - 1.1 or 2.5 etc.

    var option = $("<option />")
        .text(prefix + " " + a.text())
        .val(a.attr("href"))
        .appendTo("#nav select");
});

RobH · Answer 3 · 2013-03-27 09:38:30Z

Here's a fiddle: http://jsfiddle.net/RobH/Tm8cb/5

This is the script

$(function() {
    var options = [];

    // jQuery was designed to be fluent, why use appendTo('#nav select') 
    // when you can just chain an append?   
    $("<select />").appendTo("#nav")
        .append($("<option />", {
            "selected": "selected",
            "value": "",
            "text": "Navigate to..." 
    })); 

    $("#nav li").each(function() {
        var li = $(this),
            a = li.find("> a"),
            p = li.parents("li").length,
            arr = [],
            prefix = '';

        // EDIT: Changed '1' to ''.
        arr[p] = ''; // I assume what you want is one dash per level of depth?
        prefix = arr.join('-'); // Don't use new Array()

        options.push($("<option />")
            .text(prefix + " " + a.text())
            .val(a.attr("href")));
    });

    $("#nav select").append(options)
        .on('change', function() {
            var url = $(this).val();
            if (url) {
                window.location = url;
            }
            return false;
        });
});

It's mostly things that have been said before but there are 2 things to note:
The use of append over appendTo in the first part
Not using appendTo in the loop.

Update: Also, it may be better to do jQuery(document).ready(function($){ // your code here }); if you want your code to not rely on $ being available as an alias for jQuery. I checked the code for the navigation and it didn't work so I reverted to using your code from the first update.

asked	5 months ago
viewed	216 times
active	5 months ago

Clean up jQuery script

3 Answers

Your Answer

Not the answer you're looking for? Browse other questions tagged jquery or ask your own question.

Clean up jQuery script

3 Answers

Your Answer

Sign up or login

Post as a guest

Not the answer you're looking for? Browse other questions tagged jquery or ask your own question.

Related