patternjavascriptMinor
Sliders library
Viewed 0 times
sliderslibrarystackoverflow
Problem
I have completed working on my own pure JavaScript Slider library and would like to get some input from other developers. I would like to know how you would improve the code in any way.
```
( function( window, undefined ) {
function Slider(el, opts) {
/****
GLOBAL VARS
*/
var that = this;
//configurables
var opts = opts || {},
el = el,
minvalue = opts.minvalue || 0,
maxvalue = opts.maxvalue || 10,
step = opts.step || 1,
change = opts.change || null;
//create elements and variables
var dragging = false, //if dragger handle is being dragged
draggerStartPos, //the starting position (x coordinate) of dragger
lineWidth,
currentValue = 0,
range = (maxvalue - minvalue) / step, //draggable positions
fraction, //a draggable position in pxs
clientX,
clientY,
holder = document.createElement("div"),
line = document.createElement("div"),
dragger = document.createElement("div");
//the custom event that will be created
var event;
/****
PRIVATE SCOPE
*/
/*
* Check if it's a touch device
*/
var _isMobile = {
Android: function() {
return /Android/i.test(navigator.userAgent);
},
BlackBerry: function() {
return /BlackBerry/i.test(navigator.userAgent);
},
iOS: function() {
return /iPhone|iPad|iPod/i.test(navigator.userAgent);
},
Windows: function() {
return /IEMobile/i.test(navigator.userAgent);
},
any: function() {
return (_isMobile.Android() || _isMobile.BlackBerry() || _isMobile.iOS() || _isMobile.Windows());
}
};
/*
* Main initialisation function
*/
var _init = function(){
//construct interface
holder.setAttribute("class", "slider-holder");
line.setAttribute("class", "slider-line");
dragger.setAttribute("class", "slider-dragger");
line.appendChild(dragger);
holder.appendChild(line);
el.appendChild(holder);
lineWidth = line.offset
```
( function( window, undefined ) {
function Slider(el, opts) {
/****
GLOBAL VARS
*/
var that = this;
//configurables
var opts = opts || {},
el = el,
minvalue = opts.minvalue || 0,
maxvalue = opts.maxvalue || 10,
step = opts.step || 1,
change = opts.change || null;
//create elements and variables
var dragging = false, //if dragger handle is being dragged
draggerStartPos, //the starting position (x coordinate) of dragger
lineWidth,
currentValue = 0,
range = (maxvalue - minvalue) / step, //draggable positions
fraction, //a draggable position in pxs
clientX,
clientY,
holder = document.createElement("div"),
line = document.createElement("div"),
dragger = document.createElement("div");
//the custom event that will be created
var event;
/****
PRIVATE SCOPE
*/
/*
* Check if it's a touch device
*/
var _isMobile = {
Android: function() {
return /Android/i.test(navigator.userAgent);
},
BlackBerry: function() {
return /BlackBerry/i.test(navigator.userAgent);
},
iOS: function() {
return /iPhone|iPad|iPod/i.test(navigator.userAgent);
},
Windows: function() {
return /IEMobile/i.test(navigator.userAgent);
},
any: function() {
return (_isMobile.Android() || _isMobile.BlackBerry() || _isMobile.iOS() || _isMobile.Windows());
}
};
/*
* Main initialisation function
*/
var _init = function(){
//construct interface
holder.setAttribute("class", "slider-holder");
line.setAttribute("class", "slider-line");
dragger.setAttribute("class", "slider-dragger");
line.appendChild(dragger);
holder.appendChild(line);
el.appendChild(holder);
lineWidth = line.offset
Solution
Your configuration is just a bunch of variables:
This makes it far more difficult to identify accesses to the options in the rest of the code. I recommend you use an object for the options, have an object that provides all the defaults for each property. When building the options object for a given instance, you copy all of the properties from the default, then copy all of the properties from the passed-in options object. Another approach is to use the defaults as a prototype for the options object, copying properties from the passed options object.
It is generally not considered a a good idea to be frequently attaching and detaching event listeners. One small mistake and you might end up with event handlers piling up on an element, or a lifeless element, because no handler got reattached. It makes maintenance more difficult.
This code sets a new listener on the element every time. This is far more expensive than having an array of functions in your plugin, and one event listener for
Before (potentially) overwriting
var opts = opts || {},
el = el,
minvalue = opts.minvalue || 0,
maxvalue = opts.maxvalue || 10,
step = opts.step || 1,
change = opts.change || null;This makes it far more difficult to identify accesses to the options in the rest of the code. I recommend you use an object for the options, have an object that provides all the defaults for each property. When building the options object for a given instance, you copy all of the properties from the default, then copy all of the properties from the passed-in options object. Another approach is to use the defaults as a prototype for the options object, copying properties from the passed options object.
dragger.addEventListener("touchstart", _dragStart, false);
dragger.addEventListener("touchmove", _dragMove, false);
dragger.addEventListener("touchend", _dragStop, false);It is generally not considered a a good idea to be frequently attaching and detaching event listeners. One small mistake and you might end up with event handlers piling up on an element, or a lifeless element, because no handler got reattached. It makes maintenance more difficult.
el.addEventListener('change', function (e) {This code sets a new listener on the element every time. This is far more expensive than having an array of functions in your plugin, and one event listener for
onchange. When someone calls onchange, you .push the function into the change callback list. When a change event fires, you call all of the functions in the change callback array.// expose access to the constructor
window.Slider = Slider;Before (potentially) overwriting
window.Slider, you should copy it into a variable in your plugin called oldSlider. Add a noConflict method that puts back the value of window.Slider that you saved in your oldSlider, and returns your instance of Slider (whatever you had in window.Slider). This way, you can coexist with another plugin that sets window.Slider, that isn't written by such a considerate developer.Code Snippets
var opts = opts || {},
el = el,
minvalue = opts.minvalue || 0,
maxvalue = opts.maxvalue || 10,
step = opts.step || 1,
change = opts.change || null;dragger.addEventListener("touchstart", _dragStart, false);
dragger.addEventListener("touchmove", _dragMove, false);
dragger.addEventListener("touchend", _dragStop, false);el.addEventListener('change', function (e) {// expose access to the constructor
window.Slider = Slider;Context
StackExchange Code Review Q#68959, answer score: 8
Revisions (0)
No revisions yet.