current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
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

This code achieves the affect, but is it a resource friendly way of doing it?

Just to clarify: target div.fixMe and fix it to the top of the window when the user scrolls past its natural position. (I do realise there are plugins to achieve this result, but I wanted to build it myself)

Is this the best way to achieve the desired result? Is it a good idea that the whole function triggers whenever the user scrolls, or should I be splitting up code?

(function() {
var fixedElement = $('.fixMe').offset(),
    scrolled = $(window).scroll(function() {
        var winScrolled = $(this).scrollTop();
        if(winScrolled > fixedElement.top - 10) {
            $('.fixMe').css({'position': 'fixed','top' : '10px'})
        }
        else {
            $('.fixMe').css({'position': 'static'})
        }
    });
})()

any feedback/criticism welcome. I should probably get/set the width of div.fixMe to avoid display issues.

share|improve this question
up vote 3 down vote

Two things I noticed/would recommend:

  1. Cache jQuery variables such as $(this) and $('.fixMe') to prevent redundant lookups during each scroll event.
  2. Implement an event debouncer/limiter, this is because the scroll event fires a lot more than you likely think it does. There are many utils/plugins to achieve this such as:

I even wrote a very simple one: http://limit.gotsomething.com/. My site will show you why debouncing/limiting is useful.

Other than that looks good to me!

cheers, Marc.

share|improve this answer
    
Thanks Marc. While only recently making a concerted effort to learn JQuery I was conscious of many events firing. However if a project is of this scale, do I need to worry about this? – NickSheehy May 2 '12 at 12:59
    
In reality your code does so little that there isn't a great need to deal with these performance enhancements. Just keep them in mind when you are triggering events or functions a lot and when the result if large iterations or heavy DOM manipulation. – Marc Gagne May 2 '12 at 15:08
    
Caching $('.fixme') outside of the scroll event will be noticeable on IE at least. In IE7 on a decent sized page I think you might have trouble even scrolling the page by doing that lookup in the scroll event. – Bill Barry May 2 '12 at 15:54
up vote 0 down vote

Here is my code for this:

clone <div> element when original element reach the top of window on page-scroll.

function divFloater(div){
    var win = $(window);
    var divTop = div.offset().top;
    var divLeft = div.offset().left;

    win.scroll(function(){  
        var has_fixed = div.children('div').hasClass('fixed');
        var winTop = win.scrollTop();
        if ((winTop > divTop) && !has_fixed){            
            div.clone().appendTo(div).addClass("fixed");
            $('.fixed').css({'left':divLeft+'px'}).fadeIn();
        } else if ((winTop <= divTop)){
            $('.fixed').fadeOut();
            div.children('.fixed').remove();
        }
//        console.log(winTop+'|'+divTop'|'+has_fixed+);
    }); 
};

$(document).ready(function(){
    var floating_div=$('.view-aktualis-palyazatok').children('.view-header');
    divFloater(floating_div);
});

CSS code for this 

.view-header {
    position:relative;
}
.view-header.fixed {
    position:fixed;
    display:none;
    top: 0;
}

sandbox: http://jsfiddle.net/eapo/Djf3E/1/

Please feel free to catch bugs, and advices are welcome.

share|improve this answer

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.