1
\$\begingroup\$

I am PHP programmer whom only recently started to play with a bit of JS/jQuery. Because of that my code is probably something that most of You hardocding JS programmers would see as disgrace (again I am far from being one of you).

I wanted to create simple but quite customizable "tab> content" player and so I did.

I would like to hear from you about what I did wrong and how can I improve my code to bust performance and become more readable to other users. If on the way you find any bugs it will be grateful if you point me the right way to actual solution.

I never used OOP in JS before so if anyone comes with a nice OOP structure for my script It would be something nice to look in to. I am aware that I am not JS OOP ready yet so if you know nice place to learn OOP in JS, it would be great if you could point me to that spot.

Here is my code at CodePen

and repo on GitHub.

Also if you have an idea for new "must have" functionality please let me know. I might implement it in the close future.

//jQuery plugin tabster
(function($){
 $.tabjQeryPlugin = function(options) { //or use "$.fn.myPlugin" or "$.myPlugin" to call it globaly directly from $.myPlugin();
 var defaults = {
 mainWrapper: "#tabWraper",
 target: "div.box", // select all div.box
 buttons: "ul li a", // select all buttons
 buttonAttrName: "target", // attr that contin div tab box id np #box1
 activeClassName: "tabActive", //class to added to active button
 delayAfterClick: 0, // wait this time before fade
 fadeSpeed: 0, // fade with this speed
 showDefault: 1, // tab number be activated when loaded / false - no tab on load
 allowNone: false, // allow closing tabs when clicked seckound time on same button 
 autoPlay: true, // autoplay tabs
 speedPlay: 3500, // time for col to fade
 onClickStopPlay: true, // [true|false] When tabs chenge onclick stop auto play
 resumePlayAfter: 3500, //[3000] after stoping wait this amount of time (3s) and start to play again
 onContentHoverPause: true, // When hover on content will stop playing
 contentOuterWraper: "#contentOuterWraper", // We might need it to get padding-bot to reaply on to the tab element when allowNone is set to true % (responsive on window resize) not needed for auto or fixed height
 pluginActionOn: 'click mouseenter', // interact with buttons on ... [click | mouseenter | ...]
 };
 options = $.extend(defaults, options);
 function logic(){
 var objectClicked1; // holds element id of user curent interaction
 var objectClicked2; // holds element id of user previous interaction
 var objectClickTemp; // holds tempolary value that is placed in 
 var loadFirstTime = true; // placeholder for first time load check objectClicked2
 var interval; // placeholder for interval to play tabs every x secounds
 var curentTabIterator = 0; // placeholder for curent tab number
 var resumeTimeout = ""; // placeholder for timeout in witch function play() will be resumed
 var blockReshowingContentBox = true; // dont refresh content box if true
 var contentOuterWraperPaddingBottom = $(options.contentOuterWraper)[0].style.paddingBottom; // get padding-bottom value in % not in px
 var active_button_interval = ""; // placeholder for number of active button
 if(options.showDefault !== false) curentTabIterator = options.showDefault;
 function toogleClass(button, classname){
 $(options.buttons).removeClass(classname);
 button.toggleClass(classname);
 if(!state) button.toggleClass(classname);
 }
 function autoPlay(button, classname, box){ // return interval varible
 if(options.autoPlay === true){
 interval = setInterval(function(){
 var tabsCount = $(options.buttons).length; // count numbers of all tabs
 objectClickTemp = objectClicked1 = objectClicked1 = $(options.buttons).eq(curentTabIterator).attr(options.buttonAttrName);
 if(curentTabIterator > tabsCount - 1){ // reset to 0 if over the number of all tabs
 curentTabIterator = 0;
 }
 if(state === false && options.allowNone === true){ // it will stop closed tab to show on Autoplay or Resume
 blockReshowingContentBox = true;
 }else{ // allow to display box;
 blockReshowingContentBox = false;
 }
 $(options.target).hide(0); // hide all content box
 active_button_interval = $(options.buttons).eq(curentTabIterator); // get curent tab number
 toogleClass(active_button_interval, options.activeClassName); // ad remove css class
 if(blockReshowingContentBox !== false){
 state = false;
 }else{
 $(options.target).eq(curentTabIterator).delay(options.delayAfterClick).fadeIn(options.fadeSpeed);
 state = true;
 }
 curentTabIterator++;
 }, options.speedPlay);
 return interval;
 }
 }
 interator = autoPlay();
 function stopPlay(){
 clearTimeout(resumeTimeout);
 clearInterval(interval);
 }
 function resumePlayAfter(){
 resumeTimeout = setTimeout(function(){
 stopPlay();
 interator = autoPlay();
 info = "function start";
 }, options.resumePlayAfter);
 }
 $(options.target).hide(0); // hide all boxes
 if(options.showDefault !== false && loadFirstTime === true){ // run only once and and only if default tab is activated
 loadFirstTime = false; // change status
 var showDefault = $(options.target).eq(options.showDefault-1); // select default div to show
 showDefault.fadeIn(options.fadeSpeed); // show default div with fadeInEffect
 var defaultButton = $(options.buttons).eq(options.showDefault-1);
 var startingAttr = $(options.buttons).eq(options.showDefault-1).attr(options.buttonAttrName); //select default tag name to show
 var objectClicked1 = startingAttr; // first selected and marked as clicked once button
 var state = true; // change status
 toogleClass(defaultButton, options.activeClassName); // reamove from all and add class to curent button
 }else{
 var state = false; // change status
 stopPlay();
 }
 $(options.buttons).on(options.pluginActionOn, function(){ // ON CLICK OR MOUSE ENTER
 if(options.onClickStopPlay){
 stopPlay(); // onclick stop auto play; 
 if(options.resumeAfter !== false && options.onContentHoverPause !== true){
 resumePlayAfter(); // onclick stop auto play;
 } 
 }
 var button = $(this); // clicked element
 curentTabIterator = $(options.buttons).index(this) + 1; // onclick reset interval iterator position
 if(objectClicked1 != undefined){ objectClickTemp = objectClicked1 };
 $(options.target).hide(0); // hide div 
 var object = $(this).attr(options.buttonAttrName);
 if(objectClicked1 === undefined){ // if no object clicked
 objectClicked1 = $(this).attr(options.buttonAttrName); // set clisked button as curent active button
 objectClicked2 = $(this).attr(options.buttonAttrName); // set clisked button as curent active button
 }else if(objectClickTemp === undefined){ // if templary placeholder for clicked object is undefined
 objectClicked1 = $(this).attr(options.buttonAttrName);
 objectClicked2 = objectClicked1;
 }else{ // if non of the abowe assign corect values to objectClicked1 and objectClicked2 based on user interaction
 objectClicked1 = $(this).attr(options.buttonAttrName);
 objectClicked2 = objectClickTemp;
 }
 if(!options.allowNone){ // if settings don't allow for tab to be colsed (no tab open is allowed)
 $(options.target).hide(0);
 $(object).delay(options.delayAfterClick).fadeIn(options.fadeSpeed);
 toogleClass(button, options.activeClassName);
 state = true;
 }else{ // if tabs can be colsed (some tab must be open)
 if(!state && objectClicked1 === objectClicked2){ // check if tab content is curently closed (state === false) also check if user interact with same element for seckound time
 $(options.target).hide(0);
 $(object).delay(options.delayAfterClick).fadeIn(options.fadeSpeed);
 state = true; // change state to open
 toogleClass(button, options.activeClassName); // add or remove class to clicked button
 }else if(state && objectClicked1 === objectClicked2){ // check if tab content is curently open (state === true) also check if user interact with same element for seckound time
 $(object).delay(options.delayAfterClick).fadeOut(options.fadeSpeed);
 state = false; // change state to closed
 toogleClass(button, options.activeClassName);
 }else if(state && objectClicked1 !== objectClicked2){ // ... do this is tab content is open and user clicked on different element
 $(options.target).hide(0);
 $(object).delay(options.delayAfterClick).fadeIn(options.fadeSpeed);
 state = true;
 toogleClass(button, options.activeClassName);
 }else if(!state && objectClicked1 !== objectClicked2){ // ... do this is tab content is closed and user clicked on different element
 $(options.target).hide(0);
 $(object).delay(options.delayAfterClick).fadeIn(options.fadeSpeed);
 state = true;
 toogleClass(button, options.activeClassName);
 }
 }
 // function that gets contentouterwraper paddig and removes it if state === false (content box is hidden) 
 function onOffStateRemovePaddingFromContentOuterWraper(){
 if(state === true){
 $(options.contentOuterWraper).css('padding-bottom', contentOuterWraperPaddingBottom);
 }else{ 
 $(options.contentOuterWraper).css('padding-bottom', '0px');
 }
 };
 onOffStateRemovePaddingFromContentOuterWraper();
 });
 if(options.onContentHoverPause){
 var selection = "\""+options.target + ", " + options.buttons+ "\"";
 $(options.mainWrapper).hover(function(){
 stopPlay();
 }, function(){
 resumePlayAfter();
 });
 }
 }
 jQuery(document).ready(function($) {
 logic();
 });
 };
})(jQuery);
asked Jan 21, 2016 at 22:57
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Macro Observations

As written, $.myPlugin = ... is a jQuery static method, not a plugin. It seems more appropriate to write it a genuine plugin, $.fn.myPlugin = ... and invoke it on DOM element(s) with $(selector).myPlugin(...). You will then need to purge hard-coded selectors and take great care to ensure the independence of multiple invocations.

Strict mode will help avoid making certain mistakes. You may get error messages in your console.

Try passing the code through jsLint to check for unused/undeclared vars and heaps of other stuff.

Meso Observations

options.buttons is "ul li a", therefore $(options.buttons) will select all ul,li,a elements on the page, not just the ones you are interested in. Try constraining the selection with something like $(options.buttons, options.mainWrapper). Probably similar elsewhere for other selections.

logic() is an initialisation function and would more conventionally be named init().

Default tab: Typically with this kind of functionality, you would write initialisation function devoid of code that sets the initial condition. Then, as a final step when all other initialisation is complete, emulate user interaction by triggering a click event to select the default tab. This approach can save much time and many lines of code.

Micro Observations

state should be declared in the main declaration block with a good explanatory comment.

logic()'s local variable loadFirstTime appears to be unnecessary. At the point where it is tested, it will only ever be true, won't it?

resumeTimeout can be initialised simply as var resumeTimeout;, same as interval.

var active_button_interval is rather oddly named given that it is used for jQuery collection object, not a setInterval() reference.

objectClickTemp = objectClicked1 = objectClicked1 = ...!?

Look for unnecessary operations, such as :

$(options.target).hide(0);
...
if() {
 $(options.target).hide(0); // already done unconditionally several lines earlier
 ...
}

Look for simplifications, such as :

//javascript's `||` can make for some very abbreviated syntax, eg
var curentTabIterator = options.showDefault || 0;
if(state === false && options.allowNone === true) {
 blockReshowingContentBox = true;
} else {
 blockReshowingContentBox = false;
}
// can be written :
blockReshowingContentBox = !state && options.allowNone;

and :

$(options.mainWrapper).hover(function() {
 stopPlay();
}, function() {
 resumePlayAfter();
});
// can be written :
$(options.mainWrapper).hover(stopPlay, resumePlayAfter);

and :

if(objectClicked1) {
 objectClickTemp = objectClicked1;
}
var object = $(this).attr(options.buttonAttrName);
if(!objectClicked1) { // if no object clicked
 objectClicked1 = $(this).attr(options.buttonAttrName);
 objectClicked2 = $(this).attr(options.buttonAttrName);
} else if(!objectClickTemp) {
 objectClicked1 = $(this).attr(options.buttonAttrName);
 objectClicked2 = objectClicked1;
} else {
 objectClicked1 = $(this).attr(options.buttonAttrName);
 objectClicked2 = objectClickTemp;
}
// looks like it might simplify to :
var objectClickTemp = objectClicked1 || null; // local to the click handler
objectClicked1 = $(this).attr(options.buttonAttrName);
objectClicked2 = objectClickTemp || objectClicked1;

and :

if(!state && objectClicked1 === objectClicked2) {
 $(options.target).hide(0);
 $(object).delay(options.delayAfterClick).fadeIn(options.fadeSpeed);
 state = true; // change state to open
 toogleClass(button, options.activeClassName);
} else if(state && objectClicked1 === objectClicked2) {
 $(object).delay(options.delayAfterClick).fadeOut(options.fadeSpeed);
 state = false; // change state to closed
 toogleClass(button, options.activeClassName);
} else if(state && objectClicked1 !== objectClicked2) {
 $(options.target).hide(0);
 $(object).delay(options.delayAfterClick).fadeIn(options.fadeSpeed);
 state = true;
 toogleClass(button, options.activeClassName);
} else if(!state && objectClicked1 !== objectClicked2) {
 $(options.target).hide(0);
 $(object).delay(options.delayAfterClick).fadeIn(options.fadeSpeed);
 state = true;
 toogleClass(button, options.activeClassName);
}
// can be written :
if(state && objectClicked1 === objectClicked2) {
 $(object).delay(options.delayAfterClick).fadeOut(options.fadeSpeed);
} else {
 $(options.target).hide(0);
 $(object).delay(options.delayAfterClick).fadeIn(options.fadeSpeed);
}
state = (objectClicked1 === objectClicked2) ? !state : true;
toogleClass(button, options.activeClassName);

and :

//is :
function toogleClass(button, classname) {
 $(options.buttons).removeClass(classname);
 button.toggleClass(classname);
 if(!state) button.toggleClass(classname);
}
// not the same as :
function toogleClass(button, classname) {
 $(options.buttons).removeClass(classname);
 if(state) button.toggleClass(classname);
}
// ?

and :

// tests such as :
if(options.autoPlay === true) {...}
// can (often) be written :
if(options.autoPlay) {...}

and :

if(curentTabIterator > tabsCount - 1) {
 curentTabIterator = 0;
}
// can be written :
curentTabIterator = curentTabIterator % tabsCount;

and :

// jQuery objects need not be assigned in order to call their methods.
// In general, only assign if the collection is to be used more than once.
var showDefault = $(options.target).eq(options.showDefault-1);
showDefault.fadeIn(options.fadeSpeed);
// can be written :
$(options.target).eq(options.showDefault-1).fadeIn(options.fadeSpeed);

(Obvious) ... when doing simplifications, test at each stage and don't move on until it's working again.

answered Jan 22, 2016 at 16:10
\$\endgroup\$
2
  • \$\begingroup\$ You have taken some time to rewiev this script. I do appreciate this. Thank You. I will try to implement proposed changes on to my script. If you came up with anything more it would be nice to here from you again. Regards \$\endgroup\$ Commented Jan 22, 2016 at 18:55
  • \$\begingroup\$ Just a bunch of thoughts and ideas. Some suggestions of may work, some may not. Please feel free to ask if you need to know more. BTW: try my first suggestion last. Converting from static method to plugin is the hardest of all my suggestions. \$\endgroup\$ Commented Jan 22, 2016 at 20:04

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.