3
\$\begingroup\$

I am working on a sticky element that on certain point when the user scrolls down it turns into a relative element so it's not a sticky element anymore, only when the user scrolls to the top and passes over an specific element it turns again into a fixed/sticky element.

function Sticky($el){
    this.$el = $el;
    this.$stickyEl = this.$el.find('.sticky-bar-behavior');
    this.$noSticky = this.$el.find('.no-sticky');
    this.$whenSticky = this.$el.find('.when-sticky');
    this.shareStickyFooter();
    return this;
}

// this function is because I need check where is the scroll in order to
// see if the element should be fixed or relative
Sticky.prototype.toggleStickyOnInit = function($windowHeight, $windowPosition){
    var THIS = this;
    if(!($windowHeight >= $windowPosition)) {
        THIS.$el.addClass('position-relative');
        $('.when-sticky').show();
        $('.no-sticky').show();
    }
    return THIS;
}

Sticky.prototype.shareStickyFooter = function(){
    var THIS = this;
    var $windowHeight = $window.height();
    var $windowPosition = $('.sticky-bar-behavior').position().top;
    var $scrollTop = $(this).scrollTop();
    $window.on('scroll', function() {
        // THIS.toggleStickyOnInit($windowHeight, $windowPosition);
        if ($(this).scrollTop() + $windowHeight >= $windowPosition + 300) {
            requestAnimationFrame(function(){
                THIS.$el.removeClass('position-fixed');
                THIS.$el.addClass('position-relative');
                THIS.$noSticky.show();
                THIS.$whenSticky.hide();
            });
        } else if ($windowPosition >= $(this).scrollTop()) {
            requestAnimationFrame(function(){
                THIS.$el.removeClass('position-relative');
                THIS.$el.addClass('position-fixed');
                THIS.$noSticky.hide();
                THIS.$whenSticky.show();
            });
        }
    })

    return THIS;
};

At this point what I feel as if I am repeating some code. Is there a better way to do is?

\$\endgroup\$
1
\$\begingroup\$
  • Since this functionality is heavily dependent on jQuery, you might consider making this into a jQuery plug-in rather than javascript (
  • Consider putting hard-coded class names and other configuration values (i.e. 300 value for window position comparison) into configuration to make this class more re-usable.
  • Don't use $ prefix on variables ($windowHeight,$windowPosition, etc.) that don't represent jQuery collections if you are attempting to adhere to a coding standard that prefixes jQuery collection-holding variables in this manner.
  • I don't know that either of the functions make sense in a static/prototype context, as each of the functions deals with the "state" of an individual control or set of control bound to the object. This state should likely not be held on the prototype. You are actually calling the shareStickyFooter() function as one would an object constructor. Is toggleStickyOnInit() even used? If not, it should be removed.
  • shareStickyFooter() is an oddly named function. It looks to me like this function is simply defining scroll event handler. It doesn't "share" anything. attachScrollBehavior()?
  • Where is $window defined? If you are explicitly going to add a dependency to some global variable (which I assume is defined as $(window)), then you might consider passing that dependency in rather than just blindly relying on this being defined and available to this scope. I personally don't see great value in putting this in variable here at all.
  • I am not quite sure that the two different states that the control can seemingly be in are appropriately modeled. Why do you have both sticky and non-sticky DOM elements shown in initialized state when it seems the intent is to only have one showing at a time? Why does your scroll event handler have 3 possible outcomes (sticky/not sticky/?) vs. just two (sticky/not sticky)?
  • This code var $scrollTop = $(this).scrollTop(); seems ambiguous. What do you expect this to be in this context? I would think $(window).scrollTop() would be less ambiguous here.
  • Consider using $.toggle() and/or $.toggleClass() to consolidate code and pairing this with only doing toggle action when you cross the toggle "boundary". This will minimize re-rendering the DOM every single time a scroll happens. this of course means you would need to store some sort of variable on the current "state" of the component (sticky vs. non-sticky). This along with your other variables on positional information of the involved elements should likely exist on the object itself vs. object prototype. This probably means only having a single class which provides incremental behavior for stickyness (i.e. change position) - one class to toggle on and off rather than changing between two classes.
  • Do you really need public visibility on all of your properties?

Why do this:

this.$el = $el;
this.$stickyEl = this.$el.find('.sticky-bar-behavior');
this.$noSticky = this.$el.find('.no-sticky');
this.$whenSticky = this.$el.find('.when-sticky');
this.shareStickyFooter();
return this;

Rather than this?

var $stickyEl = $el.find('.sticky-bar-behavior');
var $noSticky = $el.find('.no-sticky');
var $whenSticky = $el.find('.when-sticky');
init();
return this;
  • Would the class benefit at all from validation that various jQuery collections are valid (i.e. not empty)? There is, of course, code size overhead for adding this, but it is something you might consider if you need to this class to be reusable in wider context.

Putting some of these thoughts together might yield code structure more like this:

function Sticky($el, config){
    // private variables
    var $stickyEl,
        $sticky,
        $noSticky,
        ...;

    // maybe we want public visibility to positioning or state information
    this.state = 'uninitialized';
    this.scrollTop;
    ...

    // constructor
    function init() {
        // not shown - perhaps validate passed collection $el and throw error if invalid
        // merge default and passed config values
        config = $.extend(this.defaultConfig, config);

        // populate your instance variables, also possibly validate
        $sticky = $el.find(config.stickyClass);
        ...

        // attach your scroll event handler
        $(window).on('scroll', onScroll);

        // perhaps some initial setup here like putting elements
        // into non-sticky state
        $el.toggleClass(config.stickyActiveClass, false);
        $sticky.toggle(false);
        $noSticky.toggle(true);
        this.state = 'nosticky';
    }

    // onScroll callback
    function onScroll() {
         var newState = 'nosticky';
         // not shown - calculate positional information
         if (/* sticky condition */) {
             newState = 'sticky';
         }
         // toggle state if needed
         if (this.state !== newState) {
             requestAnimationFrame(function(){
                 $el.toggleClass(config.stickyActiveClass);
                 $sticky.toggle();
                 $noSticky.toggle();
             });
             this.state = newState;
         }
    }

    // call constructor
    init();
    return this;
}

// set default configuration on prototype
Sticky.prototype.defaultConfig = {
    stickyActiveClass: 'sticky-active',
    ...
};
| improve this answer | |
\$\endgroup\$

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.