1

I've created a simple javascript class to wrap slick carousel.

What is the best way to instantiate multiple instances of this class?

I've currently got the below, which search the DOM and creates a new class instance of the component, if found. I suspect my project will grow a little and this 'root' file may because a little bloated/confusing. Is there a better way to organize it?

I'm going to be adding additional classes for further functionality, so trying to find the best approach early.

main.js

import Carousel from '../app/assets/javascripts/carousel/Carousel';

var carouselElements = Array.from(document.getElementsByClassName('slick-media'));  
  if (carouselElements) {
    carouselElements.map(function (element) {
      new Carousel(element, true, {xs: 6, md: 6, lg: 6 });
    });
  }

Carousel.js

export default class Carousel {
   constructor(element, variableWidth = false, breakpoint) {
      this.element = element;
      this.variableWidth = variableWidth;
      this.breakpoint = breakpoint

      this._init(element);

      /* Bind methods */
      this._init = this._init.bind(this);
   }

   _init() {
    this._instantiateSlick(this.element);
   }

   _instantiateSlick(element) {
    $(element).slick({
      lazyLoad: 'ondemand',
      slidesToScroll: 1,
      adaptiveHeight: true,
      useTransform: true,
      /* Default over lg breakpoint */
      slidesToShow: this.breakpoint.lg,
      /* Allow slick to calculate widths */
      variableWidth: this.variableWidth,
      responsive: [
        {
          breakpoint: 1199,
          settings: {
            slidesToShow: this.breakpoint.lg,
            slidesToScroll: 1,
            infinite: true
          }
        },
        {
          breakpoint: 991,
          settings: {
            slidesToShow: this.breakpoint.md,
            slidesToScroll: 1,
            infinite: true
          }
        },
        {
          breakpoint: 480,
          settings: {
            slidesToShow: this.breakpoint.xs,
            slidesToScroll: 1,
            infinite: true
          }
        }]
    });
   }
}
jsldnppl
  • 915
  • 1
  • 9
  • 28
  • This code seems to be perfectly valid and fine to me :) – Jonas Wilms Feb 16 '18 at 17:12
  • Don't use `map` when you're not using the array it returns, it's pointless overhead. Use one of [the many other ways to loop through arrays](https://stackoverflow.com/questions/9329446/for-each-over-an-array-in-javascript/9329476#9329476). – T.J. Crowder Feb 16 '18 at 17:15
  • Is your question about the first code block creating instances, or about the `Carousel` class? – T.J. Crowder Feb 16 '18 at 17:18
  • 1
    @T.J.Crowder - sorry should have made that clearer, it's about the main.js file, and the way it's instantiating multiple instances. Is this approach the best possible? Also, is it scalable in a big project, where there will be multiple classes, this file is going to become pretty unreadable? – jsldnppl Feb 16 '18 at 18:58

3 Answers3

2

Using a class for this seems overkill to me. You are never accessing the created objects later, as you don't you keep a reference to them, so their sole purpose seems to be to perform one immediate action: apply slick to elements with some predefined configuration. After their construction they immediately become useless.

Secondly, using a default value for a parameter is not very useful when after that you still have a parameter that turns out to be mandatory. So swap the parameter positions of breakpoint and variableWidth.

As an alternative I would suggest creating a jQuery plug-in for this:

$.fn.simpleSlick = function (breakpoint, variableWidth = false) {
    this.slick({
        lazyLoad: 'ondemand',
        slidesToScroll: 1,
        adaptiveHeight: true,
        useTransform: true,
        /* Default over lg breakpoint */
        slidesToShow: breakpoint.lg,
        /* Allow slick to calculate widths */
        variableWidth,
        responsive: [{
            breakpoint: 1199,
            settings: {
                slidesToShow: breakpoint.lg,
                slidesToScroll: 1,
                infinite: true
            }
        }, {
            breakpoint: 991,
            settings: {
                slidesToShow: breakpoint.md,
                slidesToScroll: 1,
                infinite: true
            }
        }, {
            breakpoint: 480,
            settings: {
                slidesToShow: breakpoint.xs,
                slidesToScroll: 1,
                infinite: true
            }
        }]
   });
   return this; // to allow chaining
};

And assuming that the slick plugin plays according to the rules (i.e. allows the jQuery object to match more than one element), your actual use of it becomes as easy as:

$('.slick-media').simpleSlick({xs: 6, md: 6, lg: 6 }, true);

When the class is a given

If the use of a class should be taken as a given, then you could still use the jQuery way for selecting the elements by their class attribute (since you already use jQuery for the slick plugin):

$('.slick-media').each(function () { 
    new Carousel(this, true, {xs: 6, md: 6, lg: 6 });
});

(But again, applying new without using the resulting object reveals a wrong design)

And if you want to access these Carousel objects later via the DOM element, then you could consider using the data method:

$('.slick-media').each(function () { 
    $(this).data('carousel', new Carousel(this, true, {xs: 6, md: 6, lg: 6 }));
});

For a given elem element, you can then access the corresponding Carousel instance as follows:

var carousel = $(elem).data('carousel'); // Get the Carousel instance
trincot
  • 317,000
  • 35
  • 244
  • 286
  • Thanks for the reply @trincot. I agree, in this specific instance, it isn't necessary. I was trying to provide an easy example so I could get an idea of a pattern to follow in this project. – jsldnppl Feb 16 '18 at 18:54
  • Are you saying this does not work in your case? If so, could you be so kind to update your question and provide an example that is representative? That way I might be able to provide an answer better tailored to your actual needs. – trincot Feb 17 '18 at 10:50
  • Whether one should make a jQuery plugin from it is another question, but yes it definitely should be a standalone function/method that is called once and not a `class` constructor – Bergi Feb 17 '18 at 11:03
2

You've clarified that your question is about how you're using your class (e.g., the code in main.js):

it's about the main.js file, and the way it's instantiating multiple instances. Is this approach the best possible?

There are a couple of nits to pick:

  1. You're using map to create an array you fill with undefined and never use. When you're not using the returned array, don't use map; use forEach of any of several other ways to loop over arrays and array-like structures.

  2. There's no need for the if; methods like getElementsByClassName always return a NodeList or HTMLCollection (depending on the method), even if it's empty.

Also, is it scalable in a big project, where there will be multiple classes, this file is going to become pretty unreadable?

I don't see any reason for it to become unreadable. If you want to stick to getElementsByClassName (because it can be amazingly fast; note that it doesn't exist on obsolete browsers like IE8 though):

import Carousel from '../app/assets/javascripts/carousel/Carousel';

const forEach = Function.call.bind(Array.prototype.forEach);

const hookUpClass = className => {
    forEach(document.getElementsByClassName(className), element => {
        new Carousel(element, true, {xs: 6, md: 6, lg: 6 });
    });
};

hookUpClass('slick-media');
hookUpClass('another-class');
hookUpClass('some-further-class');

Note the use of Array.prototype.forEach to do the looping for us, since it'll work on any array-like thing. That will work on any modern browser and IE9-IE11.

If you don't mind a very very very very small bit of overhead, you can use querySelectorAll (which exists even on IE8) to get a list matching all of your classes, rather than having to handle each class separately:

import Carousel from '../app/assets/javascripts/carousel/Carousel';

document.querySelectorAll('.slick-media, .another-class, .some-further-class').forEach(element => {
    new Carousel(element, true, {xs: 6, md: 6, lg: 6 });
});

I didn't use Array.prototype.forEach there because NodeList has its own forEach which does the same thing. If you need to support IE9-IE11, you'll want the polyfill, which is trivial:

if (typeof NodeList !== "undefined" &&
    NodeList.prototype &&
    !NodeList.prototype.forEach) {
    // Surprisingly, the standard `NodeList.prototype.forEach` is enumerable (as well as
    // writable and configurable) so we can just assign rather than using `defineProperty`
    NodeList.prototype.forEach = Array.prototype.forEach;
}

Or of course, use Array.prototype.forEach instead as we did with getElementsByClassName if you prefer.

In cutting-edge environments like the latest Chrome and Firefox, you could do this:

import Carousel from '../app/assets/javascripts/carousel/Carousel';

for (const element of document.querySelectorAll('.slick-media, .another-class, .some-further-class')) {
    new Carousel(element, true, {xs: 6, md: 6, lg: 6 });
}

...which relies on the NodeList being iterable. I didn't do that above because polyfilling an iterator on NodeList is harder than polyfilling forEach without knowing what transpiler you're using (if you're using one).

T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
1

You can actually shortify it to:

 Array.from(
  document.getElementsByClassName('slick-media'), 
  node => new Carousel(node, true, {xs: 6, md: 6, lg: 6 })
);
Jonas Wilms
  • 132,000
  • 20
  • 149
  • 151
  • "shorten" ;-) This also puts the Carousel objects into the resulting array, which is not the case in OP's code. – trincot Feb 16 '18 at 17:15
  • There's no purpose served by using `Array.from` here. As with the OP's use of `map`, it's just needless overhead. (It's also not clear the OP is asking about that code vs. the class itself.) – T.J. Crowder Feb 16 '18 at 17:16
  • Looking from different perspectives here, but yes it is either useful if the result is *used*, or not. – trincot Feb 16 '18 at 17:19
  • @t.j.crowder its browser support is better than `NodeList.forEach` i guess... And its more beautiful than a for loop. – Jonas Wilms Feb 16 '18 at 17:42
  • @JonasW., think of `for (const node of document.getElementsByClassName('slick-media')) { ... } ` – trincot Feb 16 '18 at 17:48
  • @JonasW. Yup! The WebIDL spec now says that _any_ array-like API (length and numeric keys), is supposed to be iterable automatically https://www.w3.org/TR/WebIDL-1/#es-iterator or they can explicitly opt in with an `Iterable` declaration in their type to also get `.forEach`, `.values`, `.keys`, and `.entries`. – loganfsmyth Feb 16 '18 at 18:23
  • @JonasW.: *"its browser support is better than NodeList.forEach i guess"* Why would you guess that? `Array.from` is quite new too. But `NodeList#forEach` is trivial to polyfill, or just use any of [several other ways](https://stackoverflow.com/questions/9329446/for-each-over-an-array-in-javascript/9329476#9329476) to loop array-like objects. – T.J. Crowder Feb 17 '18 at 08:49