3
\$\begingroup\$

I want to understand better how to make re-usable components that I can create multiple instances of, and I think the revealing module pattern comes handy, however, I'm not sure I'm implementing it right.

The code below receives a string that represents a single pre-existing DOM element (with certain specific structure) into which I initialize the animation and create a navigation panel to manually select the next slide, live working code can be seen here.

Am I making a good use of the pattern and the closure inside the module?

var Slider = function(opt){
        var slider = {}; // i'm using this object to add variables from the functions below and make them available to the other functions
        var currentSlide = 0;
        var nextSlide = 1;
        var sliderInterval;

        var _init = function(options){
            //recieve options, we need the -el- attribute
            slider.el = $(options.el); 
            slider.items = slider.el.find("[class^='main-header']");
            slider.sliderLength = slider.items.length;
            slider.offset = slider.el.children().length - slider.sliderLength;
            createNavigation();
            setSliderInterval();

        }

        var createNavigation = function(){
            //create the -buttons- to navigate between slides
            var ulSlider = slider.el.find('.slider-nav'); //position this where you want your navigation buttons to be
            var newLI = '<li></li>';
            var newLIActive = '<li class= "active-button"></li>';
            for (var i = 0; i< slider.sliderLength ; i++){
                if (i === 0){
                    ulSlider.append(newLIActive);   
                }
                else{
                    ulSlider.append(newLI);
                }
            }
            addEventListeners();
        };

        var addEventListeners = function(){
            // add event binders to the dynamically added li elements
            slider.el.find('.slider-nav').on('click', 'li', function() {
                //set this -> this will be the  to be the next slide, stop the function interval and start it again 
                nextSlide = $(this).index();
                clearInterval(sliderInterval);
                startSlider();
                setSliderInterval();
            });
        }

        var setSliderInterval = function(){
            sliderInterval = setInterval(startSlider, 4000);
        };

        var startSlider = function(){
            var mainDivs = slider.items;
            var ulSlider = slider.el.find('.slider-nav');
            //which slide comes next?
            if(nextSlide >= slider.sliderLength){
                nextSlide = 0;
                currentSlide = slider.sliderLength -1;
            }

            //animations using the eq selector
            //we first add the animation class, and then remove the previous one we don't want
            //toggle class, it results in an unwanted behaviour
            mainDivs.eq(currentSlide).find('h1').addClass('fade-out-bottom-top');
            mainDivs.eq(currentSlide).find('h1').removeClass('fade-in-top-bottom');

            mainDivs.eq(currentSlide).find('p').addClass('fade-out-left-right');
            mainDivs.eq(currentSlide).find('p').removeClass('fade-in-left-right');

            mainDivs.eq(currentSlide).find('a').addClass('fade-out-top-bottom');
            mainDivs.eq(currentSlide).find('a').removeClass('fade-in-bottom-top');

            mainDivs.eq(currentSlide).fadeOut('slow');

            mainDivs.eq(nextSlide).find('h1').addClass('fade-in-top-bottom');
            mainDivs.eq(nextSlide).find('h1').removeClass('fade-out-bottom-top');

            mainDivs.eq(nextSlide).find('p').addClass('fade-in-left-right');
            mainDivs.eq(nextSlide).find('p').removeClass('fade-out-left-right');

            mainDivs.eq(nextSlide).find('a').addClass('fade-in-bottom-top');
            mainDivs.eq(nextSlide).find('a').removeClass('fade-out-top-bottom');

            mainDivs.eq(nextSlide).delay(300).fadeIn('slow');

            //find offset of child elements to use their index to match the current slide with the selected button
            ulSlider.children().removeClass("active-button");
            ulSlider.children().eq(mainDivs.eq(nextSlide).index() - slider.offset).addClass("active-button");


            //update variables
            currentSlide = nextSlide;
            nextSlide += 1;
        };

        return {
            init: _init // reveal only the init function
        }

    }
    var headerSlider = new Slider();
    headerSlider.init({el: '#slider'});
\$\endgroup\$
1
  • \$\begingroup\$ I added jquery tag as the code is using jQuery. \$\endgroup\$
    – Tushar
    Commented Aug 21, 2017 at 10:02

1 Answer 1

2
\$\begingroup\$

From the demo page, I have to say, it looks very nice. I am very curious why you do not use the constructor to call init that seems a bit verbose for any user, neither does it look very good to use new Slider() as it is not a constructor in the strict sense, you don't even need it.

For a detailed description, please find it on MDN, where this explanation is of importance to you. If you return anything else but undefined, you will not receive a new Slider object, but just the returned object.

The object returned by the constructor function becomes the result of the whole new expression. If the constructor function doesn't explicitly return an object, the object created in step 1 is used instead. (Normally constructors don't return a value, but they can choose to do so if they want to override the normal object creation process.)

So, I honestly don't see the need to use a constructor and then having to call the init function, it seems very illogical to me.

Another thing that was not apparent to me, was that you are in need of jQuery, it is not mentioned anywhere explicitly, but you seem to refer to the $ inside the code. If I am not mistaking, jQuery prefers an IIFE to set up their plugin system.

It has the advantage that you can explicitly require jquery to be available, and it would be apparent through any use of your plugin itself, looking a bit like this:

(function($) {
  $.fn.slider = function( options ) {
    // manipulate the data
  }
}(jQuery));

more about plugin creation for jQuery can be learned here

Currently, I believe your method takes a bit to much control, and as a user of the slider function, I cannot really do a lot. I can only start the slider, I do not have any actions available to me where I could stop the slider (eg: onmouseover) or decide how fast it should slide, and though you offer the opt it is completely unused at the moment.

There is also no way to interact with the slider from a different place in your code, to for example show a specific slide on the click of a button. So I think your plugin is to much in control.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks! so, following your advice i have remove the _init function and have taken the initialization of the variables to the top of the constructor function, because of this i could not directly call the functions, because they had not been "read" at the time of the execution, so I had to change var createNavigation = function(){} to function createNavigation and so on to take the advantage of hoisting. about jquery i'm planning to strip it off and make it vanilla javascript, [here is an updated ](jsfiddle.net/2f7oupqu) version of my code, is this better structured? \$\endgroup\$ Commented Aug 21, 2017 at 20:55

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.