3
\$\begingroup\$

I am a bit of noob to jQuery so I got this jQuery code what I think can be way faster.

Does someone want to help me clean it up? Explaining why you did what would be real nice but not needed.

The working site is online here.

onResize = function() {
$('#spinner').delay(1000).fadeOut(500); 
// blog
setTimeout(function() {
    $('article .article_image').each(function(){
        var articleImageHeight = $(this).find('img').height();
        var articleContentHeight = $(this).parent('article').find(".article_content").height();
        var articleImageWidth = $(this).find('img').height();
        var articleContentWidth = $(this).parent('article').find(".article_content").height();

        $(this).height(articleContentHeight);    

        if (articleImageHeight <= articleContentHeight) {
            $(this).find('img').addClass('smallerheight');
            $(this).find('img').removeClass('biggerheight');
        } else  {
            $(this).find('img').addClass('biggerheight');
            $(this).find('img').removeClass('smallerheight');
        }
    });
}, 500);

// banner
var bannerPos = $('.banner_image');
var menuHeight = ($(".menu_container").height());

$(window).scroll(function () {
    if ($(this).scrollTop() > menuHeight + 15) {
        bannerPos.addClass("banner_position");
    } else {
        bannerPos.removeClass("banner_position");
    }
});  

var bannerHeight = ($(".banner_image").height() -10 +'px');
$(".banner").css({'height': bannerHeight});

// menu
var value = $('body').width() / (11);
$('#magic-line').css('top',value - 9);


var el, leftPos, newWidth,
    mainNav = $("menu.main_menu ul");
var magicLine = $("#magic-line");
magicLine
    .width($("menu.main_menu ul li.active a").width())
    .css("left", $("menu.main_menu ul li.active a").position().left)
    .data("origLeft", magicLine.position().left)
    .data("origWidth", magicLine.width());

$("menu.main_menu ul li a").hover(function() {
    el = $(this);
    leftPos = el.position().left;
    newWidth = el.parent().width();
    magicLine.stop().animate({
        left: leftPos,
        width: newWidth
    },500);
}, function() {
    magicLine.stop().animate({
        left: magicLine.data("origLeft"),
        width: magicLine.data("origWidth")
    },500);    
});



}
$(window).load('resize', onResize);
$(window).bind('resize', onResize);

And also the HTML if you don't understand my jQuery code:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"     "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
    <title>Made in 030</title>
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <link href="style.css" rel="stylesheet" media="all" type="text/css" />
    <link href="//maxcdn.bootstrapcdn.com/font-awesome/4.2.0/css/font-awesome.min.css" rel="stylesheet">
    <script src="//ajax.googleapis.com/ajax/libs/jquery/1.8.1/jquery.min.js"></script>
    <script src="js/myscript.js"></script>
</head>
<body>
    <div id="spinner"></div>
    <header>
        <div class="menu_container">
            <menu class="main_menu">
                <ul>
                    <li class="left" ><a href="#">Home</a></li>
                    <li class="left" ><a href="#">Over</a></li>
                    <li class="left"><a href="#">Probeerlijst</a></li>
                    <li class="logo_container">
                        <div class="logo">
                            <img src="img/logo.png" alt=""/>
                        </div>
                    </li>
                    <li class="right"><a href="#">Contact</a></li>  
                    <li class="right"><a href="#">Shop</a></li>
                    <li class="right active"><a href="#">Blog</a></li>
                    <li id='magic-line'></li>
                </ul>
            </menu>
        </div>
        <div class="banner">
            <img src="img/banner.jpg" class="banner_image"/>
            <img src="img/skyline.png" class="skyline_image"/>
        </div>
        <div class="banner_footer">
        </div>
    </header>
    <div id="container">
        <div class="sidebar left_sidebar">
            <menu class="categorie_menu">
                <ul>
                    <li><a href="#" class="active">All</a></li>
                    <li><a href="#">Products</a></li>
                    <li><a href="#">Food</a></li>
                    <li><a href="#">Venues</a></li>
                    <li><a href="#">Startups</a></li>
                    <li><a href="#">Evenementen</a></li>
                    <li><a href="#">Local heroes</a></li>
                </ul>
            </menu>
        </div>
        <div id="content">
            <div class="full_article">
                <h1>Made in 030</h1>
                <p>Donec scelerisque commodo orci sit amet commodo. Maecenas malesuada, nibh vitae vulputate tristique, libero purus molestie orci, sed consectetur mi justo quis arcu. Duis ligula purus, pulvinar eget leo id, pellentesque ultricies urna. Maecenas id malesuada tellus. Donec tristique enim nulla, et ultrices velit placerat id. Morbi porta, lacus quis hendrerit volutpat, ipsum diam porttitor leo, sit amet consequat lectus mi quis leo. Nulla et nisi vulputate, ultrices dolor vel, ullamcorper quam. Maecenas suscipit feugiat malesuada. Maecenas id malesuada tellus. Donec tristique enim.</p>
            </div>
            <article>
                <div class="article_image">
                    <img src="img/post1.jpg"/>
                </div>
                <div class="article_content">
                    <h2>Lorum ipsum</h2>
                    <p>Donec scelerisque commodo orci sit amet commodo. Maecenas malesuada, nibh vitae vulputate tristique, libero purus molestie orci, sed consectetur mi justo quis arcu. Duis ligula purus, pulvinar eget leo id, pellentesque ultricies urna. Maecenas id malesuada tellus. Donec tristique enim nulla, et ultrices velit placerat id. Morbi porta, lacus quis hendrerit volutpat, ipsum diam porttitor leo, sit amet consequat lectus mi quis leo. Nulla et nisi vulputate, ultrices dolor vel, ullamcorper quam. Maecenas suscipit feugiat malesuada. Maecenas id malesuada tellus. Donec tristique enim nulla, et ultrices velit placerat id. Morbi porta, lacus quis hendrerit volutpat, ipsum diam porttitor leo, sit amet consequat lectus mi quis leo. Nulla et nisi vulputate, ultrices dolor vel, ullamcorper quam. Maecenas suscipit feugiat malesuada. </p>
                </div>
            </article>
            <article>
                <div class="article_image">
                    <img src="img/post2.jpg"/>
                </div>
                <div class="article_content">
                    <h2>Lorum ipsum</h2>
                    <p>Donec scelerisque commodo orci sit amet commodo. Maecenas malesuada, nibh vitae vulputate tristique, libero purus molestie orci, sed consectetur mi justo quis arcu. Duis ligula purus, pulvinar eget leo id, pellentesque ultricies urna. Maecenas id malesuada tellus. Donec tristique enim nulla, et ultrices velit placerat id. Morbi porta, lacus quis hendrerit volutpat, ipsum diam porttitor leo, sit amet consequat lectus mi quis leo. Nulla et nisi vulputate, ultrices dolor vel, ullamcorper quam. Maecenas suscipit feugiat malesuada. Maecenas id malesuada tellus. Donec tristique enim nulla, et ultrices velit placerat id. Morbi porta, lacus quis hendrerit volutpat, ipsum diam porttitor leo, sit amet consequat lectus mi quis leo. Nulla et nisi vulputate, ultrices dolor vel, ullamcorper quam. Maecenas suscipit feugiat malesuada. </p>
                </div>
            </article>
        </div>
        <div class="sidebar right_sidebar">
            <div class="social_media">
                <ul>
                    <li><a href="https://www.facebook.com/madein030" class="facebook" target="blanc"></a></li>
                    <li><a href="https://www.facebook.com/madein030" class="twitter" target="blanc"></a></li>
                    <li><a href="https://www.facebook.com/madein030" class="pinterest" target="blanc"></a></li>
                    <li><a href="https://www.facebook.com/madein030" class="google" target="blanc"></a></li>
                    <li><a href="https://www.facebook.com/madein030" class="youtube" target="blanc"></a></li>
                </ul>
                <ul>
                    <li><a href="https://www.facebook.com/madein030" class="share" target="blanc"></a></li>
                </ul>
            </div>
        </div>
        <footer>
            <p>&copy; by Simplefly | <a href="http://www.simplefly.nl">Webdesign</a> by Simplefly | Algemene voorwaarden | Contact</p>
        </footer>
    </div>

</body>
</html>
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Your code is indeed a Mess. But it works, so you have that going for yourself.

When writing JS it is very important to keep the global scope clean. Try and polute it as little as possible. The less you expose, the less can go wrong. Just image having a var image in 2 pieces of code, each responding to the same event, but referencing a different image. The nightmare.

An easy way to keep the dom clean is an IIFE. This also gives use a way of dependency management. If a piece of your code uses jQuery, ask for it:

(function ($) {
    //now we can use the $ safely
})(jQuery);

Now we are sure that $ is mapped to jQuery. You also have a (very) small speed advantage because the compiler doesn't have to look up parent scopes for the jQuery object (Some people tend to really like micro-optimisations).

Now that we have succesfully made the dome a cleaner place. We can start writing. A good rule of thumb is to always define all your variables at the top of your script. this way it is very easy to see what you are using. All var statements are eventually moved to the top of a script by the compiler (useless micro-optimisation n2).

(function ($, window) {
    var $articleImages = $('article .article_image');

    $(window).on('resize', function() {
        $articleImages.each(function() {
            var $image = $(this).find('image'),
                $content = $(this).parent('article').find('.article_content');

            $(this).height($content.height());

            if ($image.height() <= $content.height()) {
                $image.addClass('smallerheight');
                $image.removeClass('biggerheight');
            } else {
                $image.addClass('biggerheight');
                $image.removeClass('smallerheight');
            }
        });
    });
})(jQuery, window);

Note that I removed the setTimeOut in your script. I'm not sure why it is there. It seems as if you are not reacting to the resize event but to the spinner is fadedOut event. And why is the spinner fadingOut? Is it waiting for something? It just seems odd to have all these delays. Why make the user wait? To look at some fancy spinner?

Other changes I made is DOM lookups. Everytime you lookup a piece of dom multiple times, place it in a variable. Note that all my jquery object variables start with a $. Makes it easier to see what is what.

I also removed the DOM lookup of all the articles outside of the setTimeOut function. Now we lookup the articles in the DOM only once.

I also encapsulated the logic of the articles in its own little closure. Why you ask? a spinner has nothing to do with articles. And they have nothing to do with a banner, the menu, ... So have them seperated. A good thing woul be to clean up the rest by yourself as a good pracice.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you verry much for your answer, it is really helpful and a nice explanation. I don't really understand everything about the ROM, so i'm gonna look that up. I only got one problem left. The part of the code that changes the image size based on the size of the text block does only work when i change the size of the screen and not when i first load the page. Gr Jesse \$\endgroup\$ Commented Dec 9, 2014 at 11:36
  • \$\begingroup\$ Thats because I added a listener to only the resize event. You can trigger this on load with $(window).trigger('resize'); \$\endgroup\$
    – Pinoniq
    Commented Dec 9, 2014 at 13:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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