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.

I'm still learning javascript and I'm building an ajax website that loads in content for each page from external files (e.g. test1.php, test2.php). I spent several hours cobbling together some code that works, but it feels really clunky. Any recommendations on how to streamline it? Or anything I am doing that is stupid and should be done differently?

JS:

$(document).ready(function() {

var hash = window.location.hash.substr(1);

var projectID = $('#topNav a').each(function(){
    var projectID = $(this).attr('id');
    if(hash==projectID){
        var toLoad = hash+'.php .content';
        $('.content').load(toLoad);
        $(this).siblings().removeClass('active');
        $(this).addClass('active');
    }                           
});

$('#topNav a').click(function(){

    var toLoad = $(this).attr('href')+' .content',
        newId = $(this).attr('rel'),        
        oldHeight = $('#shell').css("height"),      
        viewportHeight = $(window).height()

    if (!$(this).hasClass("active")) {

        $(this).siblings().removeClass('active');
        $(this).addClass('active');

        $('<div/>', {
            id: newId,
            class: 'content'
        }).css({ top: viewportHeight + "px", display: "none" }).appendTo('#shell').load(toLoad);

        $('#' + newId).show(function() {
            $(this).animate({ top: "0px", queue: false}, 600);
            $('#shell > div:first').animate({ top: "-" + oldHeight, queue: false}, 600, function() {

                $('#shell > div:first').remove()
            });
        });

        var newHash = $(this).attr('id');
        window.history.pushState(null, "", "#" + newHash);

    }

    return false;
});

window.addEventListener("popstate", function(e) {

    var hash = window.location.hash.substr(1),
        oldHeight = $('#shell').css("height"),      
        viewportHeight = $(window).height()

    var projectID = $('#topNav a').each(function(){
        var projectID = $(this).attr('id');
        if(hash==projectID){
            var toLoad = hash+'.php .content'

            $(this).siblings().removeClass('active');
            $(this).addClass('active');

            $('<div/>', {
                id: hash,
                class: 'content'
            }).css({ top: viewportHeight + "px", display: "none" }).appendTo('#shell').load(toLoad, function() {

                $(this).show(function() {
                    $(this).animate({ top: "0px", queue: false}, 600);
                    $('#shell > div:first').animate({ top: "-" + oldHeight, queue: false}, 600, function() {

                        $('#shell > div:first').remove()
                    });
                });

            });

        }                           
    });

});

});

HTML:

<nav id="topNav">
<a href="test1.php" id="test1" rel="content1" class="active">Test 1</a>
<a href="test2.php" id="test2" rel="content2">Test 2</a>
<a href="test3.php" id="test3"rel="content3">Test 3</a>
</nav>

<div id="shell">

<div id="content1" class="content">

    <p>Here is the first page</p>

</div>

</div>

CSS:

#topNav {
position: fixed; overflow: auto; top: 0; left: 0;
width: 100%; z-index: 100; background: #fff; height: 80px;
}
#topNav a { margin-right: 30px; color: #a9a9a9; }
  #topNav a.active { color: #333; }

#shell { margin: 0px; padding: 0px; width: 100%; height: 100%; }

.content { 
  position: absolute; margin: 0px; padding-bottom: 0px;
  width: 100%; height: 100%;
}
#content1 { background: #000; color: #fff; }
#content2 { background: red; }
#content3 { background: blue; }
share|improve this question

1 Answer 1

You should really combine code that is the same and put them into functions. Then call the functions.

I put your functionality into two categories.
1. Updating the tab links to show the active link. 2. Pulling the content from the specified pages.

So in order to do these I separated out the functionality into two separate functions and called them where appropriate. I took out the looping through of the tabs. If you had that in there as some sort of check to make sure the hash was valid then you may want to add that back in as another function.

After doing this one problem became obvious. You are losing the styles applied by id (#content1, #content2) when hitting the back button. This is because you are using the hash as the ID instead of the 'rel' attribute of the links as you are doing in other places.

Anyway here are my changes. These don't solve all problems (i.e. you now have global functions) but should lead you in the right direction.

    function loadContent(newId, toLoad) {

        var oldHeight = $('#shell').css("height"),
            viewportHeight = $(window).height();

        $('<div/>', {
            id: newId,
            class: 'content'
        }).css({ top: viewportHeight + "px", display: "none" }).appendTo('#shell').load(toLoad, function () {

            $(this).show(function () {
                $(this).animate({ top: "0px", queue: false }, 600);
                $('#shell > div:first').animate({ top: "-" + oldHeight, queue: false }, 600, function () {

                    $('#shell > div:first').remove()
                });
            });

        });

    }

    function setActiveTab(id) {
        $('#topNav a').removeClass('active');
        $('#' + id).addClass('active');
    }


    $(document).ready(function () {

        var hash = window.location.hash.substr(1);

        setActiveTab(hash);
        $('.content').load( hash + '.php .content');


        $('#topNav a').click(function () {

            var toLoad = $(this).attr('href') + ' .content',
                newId = $(this).attr('rel'),
                newHash = $(this).attr('id');

            if (!$(this).hasClass("active")) {

                setActiveTab(newHash);
                loadContent(newId, toLoad);

                window.history.pushState(null, "", "#" + newHash);

            }

            return false;
        });

        window.addEventListener("popstate", function (e) {

            var hash = window.location.hash.substr(1);
            setActive(hash);
            loadContent(hash, hash + '.php .content');

        });
    });
share|improve this answer
1  
+1 for the interpretation of the question. –  RTOSkit Jan 22 '13 at 20:49
    
Thanks for the feedback, I appreciate it! –  MJR Jan 22 '13 at 21:08

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.