I am working again and got this to work. Is this the best way to write this code?

var $window = $(window),
$sticky = $('#contentSideIner'),
elTop = $sticky.offset().top;

$window.scroll(function() {
$sticky.toggleClass('sticky', $window.scrollTop() > elTop);
$window.scroll(function() {
$sticky.toggleClass('stickyBottom', $window.scrollTop() > elTop + 4685);

I had such a hard time to get this function to work in the first place. For the longest time I was told it was not executing at all.

  • \$\begingroup\$ Welcome to Code Review! Could you tell us what you really want to accomplish with this code — what scrolling behaviour should the user see as a result? Could you post the relevant CSS too? And maybe your HTML as well, if it might help us understand what you are trying to accomplish? \$\endgroup\$ – 200_success Dec 4 '14 at 5:44

This can be simplified to this:

$(window).scroll(function() {
    var $sticky = $('#contentSideIner'),
        elTop = $sticky.offset().top, 
        wTop = $(window).scrollTop();
    $sticky.toggleClass('sticky',  wTop > elTop)
        .toggleClass('stickyBottom', wTop > elTop + 4685);
  1. The window object already exists, so no need to wait until it loads before attaching an event handler to it.
  2. Don't cache objects outside of the event handler unless it's really needed (consumes more memory, code is less encapsulated, makes your page susceptible to problems if layout or DOM elements change, etc...)
  3. Use jQuery chaining for multiple method calls on the same jQuery object
  4. Only use one event handler and put both .toggleClass() calls in the same event handler.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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