2
\$\begingroup\$

I am fairly new at writing jQuery plugins. Moreover, I am a huge fan of best practice.

I have written a plugin which centers an element inside its parent. I would love some feedback on how I could have written it better.

(function( $ ){

  $.fn.verticalCenter = function(options) {

    var settings = $.extend({
      min: 0, // Minimum width of the viewport for the centering to take effect
      max: 10000 // Maximum width of the viewport
    }, options);


    var center_element = function(element, parent){
      var elementHeight = element.height();
      parent.css({
        position:'relative'
      });
      element.css({
        position: 'absolute',
        top: "50%",
        marginTop:-elementHeight/2
      });
    }

    var remove_style = function(element, parent){
      parent.css({ position:'' });
      element.css({ position: '', top: '', marginTop: '' });
    }

    var determine_action = function(element, parent){

      $(window).on('load resize orientationchange', function() {

        var windowWidth = window.innerWidth

        if (windowWidth > settings.min && windowWidth < settings.max) {
          center_element(element, parent);
        }
        else {
          remove_style(element, parent);
        }

      });

    }

    return this.each(function(){

      var $elementToCenter = $(this);
      var $parent = $elementToCenter.parent();

      determine_action($elementToCenter, $parent);

    });

  };

})( jQuery );

The plugin is supposed to be responsive, and it allows the user to pass in viewport min and max values. These values determine if the centering should occur.

\$\endgroup\$
1
  • \$\begingroup\$ Could you set up a jsFiddle? Just include an example of how you see this plugin being used. \$\endgroup\$ Jun 12, 2013 at 17:28

1 Answer 1

2
\$\begingroup\$

Interesting question, from a once over:

  • Consider a 'use strict' after (function( $ ){
  • You are mixing lowerCamelCase and snake_case, you should stick to lowerCamelCase
  • I am not a fan of remove_style, it might cause all kinds of unintended consequences, it would be better to store the original values of element and parent and then restore them if we don't center.
  • I am also not a fan of having 1 listener function per element ( however, if you were to keep the original css info in a closure, then that's fine )
  • The code would probably be cleaner if you let center_element determine the parent.
  • The newlines before closing curly braces make the code a bit too sparse towards the end

I would probably try something like this:

(function($){
  'use strict';
  $.fn.verticalCenter = function(options) {

    var settings = $.extend({
      min: 0,    // Minimum width of the viewport for centering to take effect
      max: 10000 // Maximum width of the viewport
    }, options);

    var centerElement = function(element,parent){

      parent.css({ position:'relative' });
      element.css({
        position: 'absolute',
        top: "50%",
        marginTop:-element.height() / 2
      });
    };

    var determine_action = function(element){

      var parent = element.parent(),
          originalParentStyle = parent.style(['position']),
          originalElementStyle = element.style(['position','top','marginTop']);

      $(window).on('load resize orientationchange', function() {

        var windowWidth = window.innerWidth;

        if (windowWidth > settings.min && windowWidth < settings.max) {
          centerElement(element, parent);
        }
        else {
          parent.css(originalParentStyle);
          element.css(originalElementStyle);
        }
      });
    };

    return this.each(function(){
      determine_action($(this));
    });
  };
})(jQuery);
\$\endgroup\$

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.