4
\$\begingroup\$

I have been using a pattern similar to this basic structure below. Can anyone please let me know if this is a correct way to code an object-oriented widget for reuse? I am looking to make code than can also be extended easily and modular.

$(function(){
// wrap in jquery .fn wrapper so that may be called on a
function $lider(autoplay, velocity, controls){
  var $lider = [
    // props n methods, accessible through data-slider-* attributes
    { 
      settings : { 
        envokeBecause : $('[data-widget~="slider"]'),
        autoplay : autoplay,
        speed : velocity,
        showControls : controls,
        slideSize : '100%',// 25% will show 4 slides if the parent container is 100% width and so on
      },
      bindings : {
        slideRail : $('[data-function~="slide-rail"]'),
        slide : $('[data-function~="slide"]'),
        nextButton : $('[data-function~="next"]'),
        prevButton : $('[data-function~="prev"]'),
        playButton : $('[data-function~="play"]'),
        pauseButton : $('[data-function~="pause"]'),
        stopButton : $('[data-function~="stop"]')
        // attach this functionality to the DOM
      },
      methods : {
        slideNext :  function(){ slideRail.animate({left: '-=100%'}, velocity)   },
        slidePrev : function(){ slideRail.animate({left: '+=100%'}, velocity)  },
        slideRestart : function(){ slideRail.animate({left: '0%'}, velocity)  },
        slideStart :  function(){ window.$liderTimer = setInterval(slideNext, velocity) },
        slidesCount : function(){ slideRail.children().size()
        }
      }
    }
  ]
  $.each($lider, function(){
    //  iterate through all of the slider objects properties
    window.SliderProps = this;
    // set slider to be accessible to the global scope
  });
  // slider props stored as vars
  var $liderHook = SliderProps.settings.envokeBecause;
  var slideRail = SliderProps.bindings.slideRail;
  var slide = SliderProps.bindings.slide;
  var play = SliderProps.bindings.playButton;
  var next = SliderProps.bindings.nextButton;
  var prev = SliderProps.bindings.prevButton;
  var pause = SliderProps.bindings.pauseButton;
  var stop = SliderProps.bindings.stopButton;
  var slideCount = SliderProps.bindings.slideRail.children().size();
  var slideCounter = 0; 

  function slidePrev(){
    var slidePrev = SliderProps.methods.slidePrev();
  }
  function slideStop(){
    /*slideRail.stop(); */
    window.clearInterval( $liderTimer  )
  }
  function slideRestart(){
    var slidePrev = SliderProps.methods.slideRestart();
    slideRail.stop(); 
  }
  function autoPlay(){
    SliderProps.methods.slideStart(); 
  }
  function slideNext(){
    slideCounter++
    var slideNext = SliderProps.methods.slideNext();
    console.log(slideCounter)  
    console.log(slideCount)
    if (slideCounter != slideCount){
      // offset var-1 for js index compensation
      console.log('keep slidin..')
    }
    else {
      console.log('stop!')
      slideRestart();
      slideCounter = 0;
    }
  }
  // element -> event delegation type -> function()
  slideRail.hover( slideStop )
  next.click( slideNext )
  prev.click( slidePrev )
  stop.click( slideRestart )
  pause.click( slideStop )
  play.click( autoPlay )
} // close function slider()
$lider(true, 750);
}); 
\$\endgroup\$
  • \$\begingroup\$ I am really trying to understand what is happening when my function ($lider) is the same as my object that it contains such as this... -> var $lider = [{...}]; function makeGlobal() {window.$lider = this } $.each($lider, makeGlobal). And also if anyone can get what I am trying to do and help me out with improvements. That would be simply fantastic! \$\endgroup\$ – NicholasAbrams Aug 8 '14 at 1:42
3
\$\begingroup\$

I'd suggest taking a look at jquery-boilerplate, especially the jqueryUI widget portion.

I started refactoring your code in a true $widget manner. It's not completely functional but should point you in the right direction.

;(function ( $, window, document, undefined ) {
  $.widget("yourCustomNamespace.slider", {

    /*
     * Options to be used as defaults:
     */

    options: {
      autoplay: true,
      controls: true,
      duration: 750,
      slideSize: '100%',
      selectors: {
        slideRail: '[data-function~="slide-rail"]',
        slide: '[data-function~="slide"]',
        next: '[data-function~="next"]',
        prev: '[data-function~="prev"]',
        play: '[data-function~="play"]',
        pause: '[data-function~="pause"]',
        stop: '[data-function~="stop"]'
      }
    },

    /*
     * Private Methods:
     */

    _create: function() {
      this._setOption("current", 0);
      this._setOption("total", this.element.children().length);
      this._on(this.element, {
        "hover " + this.options.selectors.slideRail: $.proxy(this.stop, this),
        "click " + this.options.selectors.next: $.proxy(this.next, this),
        "click " + this.options.selectors.prev: $.proxy(this.prev, this),
        "click " + this.options.selectors.stop: $.proxy(this.stop, this),
        "click " + this.options.selectors.pause: $.proxy(this.pause, this),
        "click " + this.options.selectors.play: $.proxy(this.play, this)
      });
      if(this.options.autoplay) {
        this.start();
      }
    },
    _destroy: function() {
      this._off(this.element, "click hover");
    },
    _setOption: function(key, value) {
      switch (key) {
        case "current":
          // Update internal object ...
          this.options[key] = value;
          // Update DOM ...
          // this.element.find(this.options.selectors.slideRail.animate({...}, this.options.duration) ...
          break;
        default:
          this.options[key] = value;
          break;
      }
      return this._super("_setOption", key, value);
    },

    /*
     * Public Methods:
     */

    next: function() {
      // TODO: Account for overflow, this.options.total
      this._setOption("current", this.options.current + 1);
    },
    prev: function() {
      // TODO: Account for underflow, this.options.total
      this._setOption("current", this.options.current -1);
    },
    restart: function() {
      this._setOption("current", 0);
    },
    start: function() {
      this.interval = window.setInterval($.proxy(this.next, this), this.options.duration);
    },
    stop: function() {
      window.clearInterval(this.interval);
    },
    getCount: function() {
      return this.options.total;
    }
  });
})( jQuery, window, document );

Reference

There are a couple of things regarding reusability going on in here:

  1. options are your default parameters that can be overwritten on initializing a new widget instance, e.g. $('[data-widget~="slider"]').slider({autoplay: false}); would overwrite the default of autoplay: true

  2. Event binding is handled internally via this._on()

  3. Ideally the widget would manage its own application state logic via this._setOption(). That way you can distinguish between logic and DOM interaction. Whenever a value changes, you update the DOM accordingly.

  4. You can call public methods from other places via the automatic interface the widget provides, e.g. $('[data-widget~="slider"]').slider("next")

\$\endgroup\$
2
\$\begingroup\$

I don't write , even less so , but one thing that strikes me is your naming.

$lider
$liderProps
$liderHook

I find that's a bit of a [perhaps mild] abuse of the $ dollar sign. I would expect these names to be:

$slider
$sliderProps
$sliderHook

Which would be consistent with some of the rest of the identifiers you have:

var stop = SliderProps.bindings.stopButton;
var slideCount = SliderProps.bindings.slideRail.children().size();
var slideCounter = 0;

Instead of

var $top = SliderProps.bindings.stopButton;
var $lideCount = SliderProps.bindings.slideRail.children().size();
var $lideCounter = 0;

I wouldn't treat the $ as [sometimes, sometimes not] part of the identifier.

\$\endgroup\$
  • \$\begingroup\$ Yes what your saying is correct, I knew I was gonna here about that part. \$\endgroup\$ – NicholasAbrams Aug 8 '14 at 1:34
2
\$\begingroup\$

First, read @MatsMug's answer. Personally, I think that your final exported function is okay as $lider() (it's cute, almost), but your internal stuff should still be named slider, sliderProps, etc.

  • $lider (the variable) is a one-element array. Always. Why would you call $.each on it?

  • jQuery plugins typically will take settings objects and then $.extend them. In some cases, they will also take certain parameters for convenience. You could rewrite like the following:

    function $lider(settings) {
        var defaults = {
            envokeBecause: $('[data-widget~="slider"]'),
            autoplay: true,
            speed: 400,
            showControls: true,
            slideSize: '100%'
        };
        settings = $.extend({}, defaults, settings);
        ...
    
  • What is envokeBecause? I assume you mean invokeBecause, but I still am not sure what it's supposed to be. It seems like it's the selector for your slider widget, in which case you can just name it selector or sliderSelector or something.

  • You've got a setting named speed (or velocity depending on where in the code you look... that should be made consistent!), but that's misleading. speed actually refers to duration, so consider naming it that.

  • $liderTimer would do better as a local variable instead of being global scope.

  • All of your methods should be using $lider.settings.speed (or SliderProps.settings.speed). Or rather, .duration, assuming you've renamed it.

  • slidesCount() doesn't do anything. It needs to return it. Furthermore, .size() is deprecated and should be replaced with .length (NOT a function). Finally, for consistency, the closing brace should be on the same line.

  • Why are you assigning var slidePrev in slidePrev() and slideNext()...?

  • When binding your events, it's recommended that you use .on("click", ... instead of .click( ....

\$\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.