Have difficulty understanding a logic in JS slider script

JavaScript
1

The source script that I am analyzing is here.

var itemClassName = "carousel__photo";
items = d.getElementsByClassName(itemClassName),
totalItems = items.length,
slide = 0,
moving = true;

// Set classes
function setInitialClasses() {
  // Targets the previous, current, and next items
  // This assumes there are at least three items.
  items[totalItems - 1].classList.add("prev");
  items[0].classList.add("active");
  items[1].classList.add("next");
}

To understand better I initially assume only 3 slides. In that case:

items[totalItems - 1] → This will be items[2].
So why on the third item we are putting calls prev

Currently, only the first one(items[0]) is visible.

3

My understanding is that even though it’s not visible, it allows the browser to preload the image so that you don’t have to wait for it on your next interaction.

1 Like
4

If you are on a continuous carousel with only 3 items then the 3rd item will be the previous item to the initial first item. Imagine you are on the first slide and you press previous then that will bring in the last slide in the group (number 3).

1 Like
5

Ok so logically although on the front end it seems to be a linear motion, logically it is a closed-loop and nothing is last or perhaps first, but tied in a logical closed circle where even the last item, which is yet not visible, becomes previous to the current first visible item.

1 Like
6

There is one more question related to the link that I posted above.

// Next navigation handler
function moveNext() {
  // Check if moving
  if (!moving) {
    // If it's the last slide, reset to 0, else +1
    if (slide === (totalItems - 1)) {
      slide = 0;
    } else {
      slide++;
    }
    // Move carousel to updated slide
    moveCarouselTo(slide);
  }

Initially moving is set to binary true that means:
!moving = false

But neither is moving a keyword in JS how come something that is assigned an initial value true will even be set to false automatically?

Question #2 →

image
image920Ă—331 28.8 KB

I have copy-pasted code on my local text editor.

Only itemClassName is defined with var, but not other things. Is there anything default or some coding style is there, which I do not know. the slide is a variable there but yet not given modifier var

7

Not sure what you are asking but its set to false a few times in the script and is used to stop you clicking the buttons while it is moving.

  function disableInteraction() {
    moving = true;
    setTimeout(function(){
      moving = false
    }, 500);
  }

To my untrained eyes that looks like an error.

If you attempt to read the value of an undeclared variable, JavaScript generates an error. If you assign a value to a variable that you have not declared with var , JavaScript implicitly declares that variable for you. Note, however, that implicitly declared variables are always created as global variables, even if they are used within the body of a function. To prevent the creation of a global variable (or the use of an existing global variable) when you meant to create a local variable to use within a single function, you must always use the var statement within function bodies. It’s best to use var for all variables, whether global or local. (The distinction between local and global variables is explored in more detail in the next section.)

Maybe @Paul_Wilkins can confirm :slight_smile:

1 Like
8

Thanks Paul. That all makes a lot of sense. Sometimes what looks like an undeclared variable is read, but it’s really not undeclared because later on in the code that variable might be declared. That all gets to be rather confusing.

That’s why it’s preferred these days to not use var, and instead to use const or let. That way your code ends up being easier to understand and work with.

Because const doesn’t let you reassign the value (although you can change values in an array and in objects), it’s preferred by far to use const instead of let. That way the code ends up being a lot more robust and secure than it was before.

Let’s take a look at the code from https://codepen.io/marcusmichaels/pen/yGGoLM

It starts with:

  var itemClassName = "carousel__photo";
      items = d.getElementsByClassName(itemClassName),
      totalItems = items.length,
      slide = 0,
      moving = true;

An unintended error has occurred here. He ended the “carousel_photo” line with a semicolon, when based on the indent and the commas at the end of the other lines, he intended to use a comma instead to define local variables.

What has really happened instead is that items, totalItems, slide, and moving have all been defined as global variables.

We can refactor this code to instead make them all local variables as intended, by replacing that semicolon with a comma instead.

  var itemClassName = "carousel__photo",
      // items = d.getElementsByClassName(itemClassName);
      items = d.getElementsByClassName(itemClassName),
      //...
      totalItems = items.length,
      slide = 0,
      moving = true;

Because that type of mistake is very easy to make, it’s preferred to avoid defining variables in a list, and define them one at a time, on separate lines.

  var itemClassName = "carousel__photo";
  var items = d.getElementsByClassName(itemClassName);
  var totalItems = items.length;
  var slide = 0;
  var moving = true;

Another benefit of defining variables on separate lines is that the code is also easier to refactor and make changes to as well.

Speaking of which, we can update the code to use const instead of var, which I’ll delve into in my next post :slight_smile:

3 Likes
9

This makes much sense to me. Thanks.

10

I got it the whole script needs to be viewed in its totality instead of just focusing on a fragment of script.

11

Checking the code for which variables change, those are moving, oldPrevious, oldNext, newPrevious, newNext, and slide. All other variables can be safely and immediately switched over to use const. The variables that change can temporarily use let instead.

  const itemClassName = "carousel__photo";
  const items = d.getElementsByClassName(itemClassName);
  const totalItems = items.length;
  let slide = 0;
  let moving = true;

Using let tends to be a bandaid solution though. Things tend to be better we can can remove the need to use let at all. Typically we can do that by using an object to store the existing state of things.

There are three sets of variables that want to be focused on here, slide, moving, and the prev/next ones.

Improving the slide variable

The slide variable is used in three places, the moveCarousel function where it is the function parameter, and in the moveNext and movePrev functions. After some searching around I find that the moveNext and movePrev functions are event handlers. I prever to make that more explict, by using functions called moveNextHandler and movePrevHandler.

  function moveNextHandler(evt) {
    moveNext();
  }
  function movePrevHandler(evt) {
    movePrev();
  }
//...
    next.addEventListener('click', moveNextHandler);
    prev.addEventListener('click', movePrevHandler);

By keeping the handler function nice and simple, we can figure out how to pass the slide variable to it.

The moveNext function not only reads from the slide variable, it also changes it. We can pass the slide variable to the moveNext function, and return the slide variable, letting us do the update in the handler function itself.

  function moveNextHandler(evt) {
    slide = moveNext(slide);
  }
  function movePrevHandler(evt) {
    slide = movePrev(slide);
  }

Now that those move functions are using their own separate slide variable, the only involvement that we need to worry about is in the handler functions. We can now put that use of the slide variable into an object for easy reference, and everything in regard to that slide variable is tidied up.

  const carousel = {
    slide: 0
  };
//...
  function moveNextHandler(evt) {
    carousel.slide = moveNext(carousel.slide);
  }
  function movePrevHandler(evt) {
    carousel.slide = movePrev(carousel.slide);
  }

Improving the moving variable

Another way to look at what we have done is to ensure that the move functions are being controlled based entirely on their function parameters. Their behaviour does change currently based on another variable, and that is the moving one. That for good behaviour should also be passed to the functions.

I will also rename it in the move functions to be isMoving too, for even better clarity.

We can give that moving variable to the move functions in the handler, and have the move functions make good uses of that.

  function moveNextHandler(evt) {
    carousel.slide = moveNext(carousel.slide, moving);
  }
  function movePrevHandler(evt) {
    carousel.slide = movePrev(carousel.slide, moving);
  }
//...
  function moveNext(slide, isMoving) {

    // Check if moving
    if (!isMoving) {
//...
  }
  function movePrev(slide, isMoving) {

    // Check if moving
    if (!isMoving) {
//...
  }

We can simplify things further by putting isMoving in the carousel object, and only use that one object in the move functions instead.

That a lot of changes, so let’s split them up into renaming, moving into object, and simplifying the code.

Rename moving to isMoving

This simple rename of the variable helps to ensure that we have a consistent and easily understood variable being used all throughout the code.

  // let moving = true;
  let isMoving = true;
//...
  function moveNextHandler(evt) {
    // carousel.slide = moveNext(carousel.slide, moving);
    carousel.slide = moveNext(carousel.slide, isMoving);
  }
  function movePrevHandler(evt) {
    // carousel.slide = movePrev(carousel.slide, moving);
    carousel.slide = movePrev(carousel.slide, isMoving);
  }
//...
  function disableInteraction() {
    // moving = true;
    isMoving = true;

    setTimeout(function(){
      // moving = false;
      isMoving = false;
    }, 500);
  }
//...
    // Check if carousel is moving, if not, allow interaction
    // if(!moving) {
    if(!isMoving) {
//...
  function initCarousel() {
    setInitialClasses();
    setEventListeners();

    // Set isMoving to false now that the carousel is ready
    // moving = false;
    isMoving = false;
  }

Not only does that make it easier to understand without context, but it gives us a good overview of all the places that use the isMoving variable, making it easier to make plans about the next changes to be made.

Move isMoving into object

We can now move isMoving into the carousel object, and make notes about future changes that we want to make.

  const carousel = {
    slide: 0,
    isMoving: true
  };
  // let isMoving = true;

  function moveNextHandler(evt) {
    // carousel.slide = moveNext(carousel.slide, isMoving);
    carousel.slide = moveNext(carousel.slide, carousel.isMoving);
  }
  function movePrevHandler(evt) {
    // carousel.slide = movePrev(carousel.slide, isMoving);
    carousel.slide = movePrev(carousel.slide, carousel.isMoving);
  }

Here we will want to combine both of those variables together, so that it is only the carousel object being passed to the function.

  function disableInteraction() {
    carousel.isMoving = true;

    setTimeout(function(){
      carousel.isMoving = false;
    }, 500);
  }

Here we will want to find out if we can pass the carousel to disableInteraction, so that it can be updated via the function parameter instead.

  function moveCarouselTo(slide) {

    // Check if carousel is moving, if not, allow interaction
    // if(!isMoving) {
    if(!carousel.isMoving) {

Here we will want to pass the carousel object to the function and use that instead.

  function initCarousel() {
    setInitialClasses();
    setEventListeners();

    // Set moving to false now that the carousel is ready
    carousel.isMoving = false;
  }

We shouldn’t need to do anything else with isMoving in the init function.

What do we have in our TODO list now?

  • combine handler variables
  • pass carousel to disableInteraction
  • pass carousel to moveCarouselTo

We’ll take care of those in the next post, and finish off by examining any remaining let statements.

2 Likes
12

Our TODO list is as follows:

  • combine handler variables
  • pass carousel to disableInteraction
  • pass carousel to moveCarouselTo
  • examine remaining let variables

combine handler variables

This part is easy. Instead of passing two function parameters, we pass only one that contains them both.

  function moveNextHandler(evt) {
    // carousel.slide = moveNext(carousel.slide, carousel.isMoving);
    carousel.slide = moveNext(carousel);
  }
  function movePrevHandler(evt) {
    // carousel.slide = movePrev(carousel.slide, carousel.isMoving);
    carousel.slide = movePrev(carousel);
  }
//...
  // function moveNext(slide, isMoving) {
  function moveNext({slide, isMoving}) {
//...
  // function movePrev(slide, isMoving) {
  function movePrev({slide, isMoving}) {

Pass carousel to disableInteraction

The carousel object can now be passed to the disableInteraction function.

  // function disableInteraction() {
  function disableInteraction(carousel) {
    //...
  }
//...
      // disableInteraction();
      disableInteraction(carousel);

Pass carousel to moveCarouselTo

The moveCarouselTo function uses both slide and isMoving, so we can just use the one carousel parameter with that function instead.

Let’s start by passing both slide and isMoving to the moveCarouselTo function.

  // function moveCarouselTo(slide) {
  function moveCarouselTo(slide, isMoving) {

    // Check if carousel is moving, if not, allow interaction
    // if(!carousel.isMoving) {
    if(!isMoving) {
//...
      // moveCarouselTo(slide);
      moveCarouselTo(slide, isMoving);

In order to pass carousel to the moveCarouselTo function, we want to update the carousel slide property before doing that. We have a couple of approaches that could be taken here. We could update the carousel object before calling moveCarouselTo, or we could move that moveCarouselTo call out of the moveTo function and into the event listener. That latter option is what I’ll do here today.

Currently the moveNext function has two responsibilities, to figure out the next slide, and to move to that next slide. That breaches the single-responsibility principle. We should split up the function so that it has only one main thing to take care of instead.

I’ll copy some of what moveNext does to a separate nextSlide function, and move the moveCarouselTo function into the event handler instead.

  function nextSlide({slide, isMoving}) {

    // Check if moving
    if (!isMoving) {

      // If it's the last slide, reset to 0, else +1
      if (slide === (totalItems - 1)) {
        slide = 0;
      } else {
        slide++;
      }
    }
    return slide;
  }

We can now replace moveNext with two different function calls in the event handler.

  function moveNextHandler(evt) {
    carousel.slide = nextSlide(carousel);
    moveCarouselTo(carousel.slide, carousel.isMoving);
  }

and the old moveNext function no longer being used, can be deleted.

We can do similar with the movePrev function too.

  function movePrevHandler(evt) {
    // carousel.slide = movePrev(carousel);
    carousel.slide = prevSlide(carousel);
    moveCarouselTo(carousel.slide, carousel.isMoving);
  }
//...
  function prevSlide({slide, isMoving}) {

    // Check if moving
    if (!isMoving) {

      // If it's the first slide, set as the last slide, else -1
      if (slide === 0) {
        slide = (totalItems - 1);
      } else {
        slide--;
      }
    }
    return slide;
  }

We can now combine the two moveCarouselTo function parameters together into the one carousel object.

    carousel.slide = nextSlide(carousel);
    // moveCarouselTo(carousel.slide, carousel.isMoving);
    moveCarouselTo(carousel);
//...
    carousel.slide = prevSlide(carousel);
    moveCarouselTo(carousel);
//...
  // function moveCarouselTo(slide, isMoving) {
  function moveCarouselTo({slide, isMoving}) {

Use a guard clause

I also noticed in the nextSlide and prevSlide functions an easy improvement. Those functions are quite nested with several if statements. We can improve on that by exiting the function early, using what’s called a guard clause.

  function nextSlide({slide, isMoving}) {
    // if (!isMoving) {
    if (isMoving) {
      return slide;
    }
    //...
    return slide;
  }

  function prevSlide({slide, isMoving}) {
    // if (!isMoving) {
    if (isMoving) {
      return slide;
    }
    //...
    return slide;
  }

I also see that we can add a similar guard clause to the moveCarouselTo function as well.

  function moveCarouselTo({slide, isMoving}) {

    if(isMoving) {
      return;
    }
//...
  }

Those guard clauses don’t need to be in the nextSlide, prevSlide, and moveCarouselTo functions. Instead they can be removed from there and put only in the event handlers.

  function moveNextHandler(evt) {
    if(carousel.isMoving) {
      return;
    }
    carousel.slide = nextSlide(carousel);
    moveCarouselTo(carousel);
  }
  function movePrevHandler(evt) {
    if(carousel.isMoving) {
      return;
    }
    carousel.slide = prevSlide(carousel);
    moveCarouselTo(carousel);
  }
//...
  function moveCarouselTo({slide, isMoving}) {
    // if(isMoving) {
    //   return;
    // }
    ...
  }
  function nextSlide({slide, isMoving}) {
    // if (isMoving) {
    //   return slide;
    // }
    ...
  }
  function prevSlide({slide, isMoving}) {
    // if (isMoving) {
    //   return slide;
    // }
    ...
  }

Examine remaining let variables

The last thing that we had on the TODO list is to investigate any remaining let statements.

    let newPrevious = slide - 1;
    let newNext = slide + 1;
    let oldPrevious = slide - 2;
    let oldNext = slide + 2;

We can instead use some simple objects called prev and next to store that information.

    const prev = {
      new: slide - 1,
      old: slide - 2
    };
    const next = {
      new: slide + 1,
      old: slide + 2
    };

We can now rename things to move them over to the prev and next objects.

      // if (newPrevious <= 0) {
      if (prev.new <= 0) {
        // oldPrevious = totalItems - 1;
        prev.old = totalItems - 1;
      // } else if (newNext >= totalItems - 1) {
      } else if (next.new >= totalItems - 1) {
        // oldNext = 0;
        next.old = 0;
      }

      if (slide === 0) {
        // newPrevious = totalItems - 1;
        prev.new = totalItems - 1;
        // oldPrevious = totalItems - 2;
        prev.old = totalItems - 2;
        // oldNext = slide + 1;
        next.old = slide + 1;
      } else if (slide === totalItems - 1) {
        // newPrevious = slide - 1;
        prev.new = slide - 1;
        // newNext = 0;
        next.new = 0;
        // oldNext = 1;
        next.old = 1;
      }

      // Based on the current slide, reset to default classes.
      // items[oldPrevious].className = itemClassName;
      items[prev.old].className = itemClassName;
      // items[oldNext].className = itemClassName;
      items[next.old].className = itemClassName;

      // Add the new classes
      // items[newPrevious].className = itemClassName + " prev";
      items[prev.new].className = itemClassName + " prev";
      // items[slide].className = itemClassName + " active";
      items[slide].className = itemClassName + " active";
      // items[newNext].className = itemClassName + " next";
      items[next.new].className = itemClassName + " next";

Those last remaining let statements are no longer being used, and so can be removed.

    // let newPrevious = slide - 1;
    // let newNext = slide + 1;
    // let oldPrevious = slide - 2;
    // let oldNext = slide + 2;

Summary

This has been a somewhat in-depth look at using const and let instead of var, and removing the need for let. It can result in refactoring several changes to the code, but almost all of the time those changes result in improvements to the code and its structure.

Here’s a forked version of the codePen code that has all of the above improvements made to it. I also used the format javascript option and JSLint to help make the code consistent too.

2 Likes
13

I am studying your post. In the interim, I have a question. Is this like the __construct function in PHP that is fired instantly:

!(function (d)  {
})(document);
14

That’s an IIFE (immediately invoked function expression) that helps to secure and contain the functions and variables inside of of it, so that they don’t end up polluting the global namespace.

I prefer using the following IIFE code instead:

(function iife(d) {
    // ...
}(document));

That has three differences.

  • The exclamation mark is an ugly hack that has no benefit here at all.
  • The function is named to help make it clearer what it’s used for.
  • Instead of the whole parenthesised function being called, it’s the function being called itself, with the parenthesis around the whole lot being used to let us immediately invoke the function without causing a syntax error.
2 Likes
15

Hi there @Paul_Wilkins, I am facing difficulty in understanding this part →

image
image879Ă—595 50.9 KB

certainly, some fundamental understanding is missing.

16

I don’t think so. The initCarousel function is more like that.

The nextSlide function gets the index value of the next image that will be slid to.
The carousel.slide = ... part updates the slide property, with the index value of the next image that will be slid to.

The moveCarouselTo(carousel) part moves the carousel to the image that will be slid to.

In effect the two lines do two things. They figure out where to move, and then does the move.

1 Like
17

So, we should make the code easier to understand in that way.

What would be preferable is to have code that looks like this:

const nextSlideIndex = nextSlide(currentSlideIndex);
moveCarouselTo(nextSlideIndex);

We can start from the bottom up to make those changes. The moveCarouselTo function doesn’t need to have carousel as a function parameter. That was me getting far too finickity about the function parameters all being responsible for their behaviour.

The moveCarouselTo function doesn’t update any of the carousel information. We can instead define the item class name and the items directly as variables, removing them from the carousel object.

    // carousel.itemClassName = "carousel__photo";
    const itemClassName = "carousel__photo";
    // carousel.items = d.getElementsByClassName(carousel.itemClassName);
    const items = d.getElementsByClassName(itemClassName);
//...
        // Test if carousel has more than three items
        const lastItemIndex = items.length - 1;
//...
            // itemClassName = carousel.itemClassName;
            // carousel.items[prev.old].className = itemClassName;
            items[prev.old].className = itemClassName;
            // carousel.items[next.old].className = itemClassName;
            items[next.old].className = itemClassName;

            // carousel.items[prev.new].className = itemClassName + " prev";
            items[prev.new].className = itemClassName + " prev";
            // carousel.items[slide].className = itemClassName + " active";
            items[slide].className = itemClassName + " active";
            // carousel.items[next.new].className = itemClassName + " next";
            items[next.new].className = itemClassName + " next";
//...
        // const lastItemIndex = carousel.items.length - 1;
        const lastItemIndex = items.length - 1;

Now that the moveCarouselTo function has only the slide variable and the isMoving, that’s responsible for causing change, we can ensure that’s expressed more clearly in the function parameters.

I’ll deal with the disableInteraction function first, which uses the isMoving property to decide what to do.

    // function disableInteraction(carousel) {
    function disableInteraction() {
        //...
    }
    function moveCarouselTo(slide) {
        // disableInteraction(carousel);
        disableInteraction();

Now that we don’t need to pass that carousel variable to disableInteraction, we can update the moveCarouselTo function parameters too so that slide is passed to the function, instead of being retrieved from the carousel object.

    // function moveCarouselTo(carousel) {
    function moveCarouselTo(slide) {
        disableInteraction();

        // const slide = carousel.slide;
        //...
    }
//...
        carousel.slide = nextSlide(carousel);
        // moveCarouselTo(carousel);
        moveCarouselTo(carousel.slide);
//...
        carousel.slide = prevSlide(carousel);
        // moveCarouselTo(carousel);
        moveCarouselTo(carousel.slide);

The nextSlide and prevSlide functions don’t need to have the carousel object passed to them either. It is more appropriate for the slide to be given as a function parameter.

    // function nextSlide(carousel) {
    function nextSlide(slide) {
        // const slide = carousel.slide;
        //...
    }
    function prevSlide(carousel) {
        // const slide = carousel.slide;
        //...
    }
//...
        // carousel.slide = nextSlide(carousel);
        carousel.slide = nextSlide(carousel.slide);
        moveCarouselTo(carousel.slide);
//...
        // carousel.slide = prevSlide(carousel);
        carousel.slide = prevSlide(carousel.slide);
        moveCarouselTo(carousel.slide);

We can now work on making the handler easier to understand:

        // carousel.slide = nextSlide(carousel.slide);
        const slide = nextSlide(carousel.slide);
        // moveCarouselTo(carousel.slide);
        moveCarouselTo(slide);
        carousel.slide = slide;

The next handler now looks much easier to understand:

    function moveNextHandler() {
        if (carousel.isMoving) {
            return;
        }
        const slide = nextSlide(carousel.slide);
        moveCarouselTo(slide);
        carousel.slide = slide;
    }

Here is the updated version of the code:

1 Like
18

[offtopic]

There’s a serious flaw in that slider logic (not Paul’s code as that would be impossible) but in the way that it reverses the slides. If you slow it down and slide left you get a blank screen while the item slides rather than the current one sliding off and the next one sliding in together smoothly.

If you press the right button they slide together ok until you get to the last one and then it goes blank until it starts again. However if you press the left button only one slides at a time and the screen is blank to the side (apart from one slide).

You can see it here where I have slowed down the slide in Pauls’ example.

I know this was a learning exercise anyway but its something that should be looked at in production as the effect is not very nice. The slides need to be able to slide in continuously left or right in pairs.

Compare it to the smooth sliding action in my rather pathetic previous attempt.

(Don’t look at the jS Paul it will burn your eyes out :slight_smile: I will get around to tidying it up when I have time based on how you broke down this thread ;))

[/offtopic]

1 Like
19

With the example that I was working on, the refactoring that I was doing deliberately made no change to the actual behaviour of the code. That is why the sins of the father are still plaguing us.

However, the updated code should be easier to understand and make improvements to.

Placing a console.log statement in the event handler lets us see what’s going on.

        console.log(slide);
        moveCarouselTo(slide);

Having different cat images will help too, so I’ve updated the HTML code to use slightly different image sizes, which gives different cat images.

      <img class="carousel__photo initial" src="http://placekitten.com/1600/900">
      <img class="carousel__photo" src="http://placekitten.com/g/1601/900">
      <img class="carousel__photo" src="http://placekitten.com/1600/901">
      <img class="carousel__photo" src="http://placekitten.com/g/1601/901">
      <img class="carousel__photo" src="http://placekitten.com/1599/899">

The page starts by showing us slide 0. The next button correctly takes us to 1, then 2, then 3, then 4, then has a display problem when looping back around to 0, as the 4th image doesn’t slide, it get shoved to the right, and slide 4 fades out in the same place as slide 0, which is quite unsettling.

The prev button is even worse. When going backwards the slide we see is one more than the slide we are actually on. On a fresh page load when we press the prev button, the slide number updates from 0 to 4, but we are still shown number 0. Pressing prev again updates slide to 3 but we are shown slide 4, and the animation just blanks the current slide, with the new one sliding and fading in. We should have the current one sliding out too.

So in all I agree, there are a lot of problems with the behaviour of that carousel.

It won’t be today though that I’ll be taking a deeper look.

2 Likes
20

Good news is that it can be fixed with a little CSS :slight_smile:

Specifically adding a left position to the css here.

.carousel__photo {
  opacity: 0;
  position: absolute;
  top:0;
  left:0;/* this is important*/
  width: 100%;
  margin: auto;
  padding: 1rem 4rem;
  z-index: 100;
  transition: transform 2.5s, opacity 2.5s, z-index 2.5s;
}

The author made a mistake in that there was no left position set which means it would have been auto positioned (e.g. where it would have been in the normal flow had it not been absolutely positioned). This put all the non active slides initially to the right of the first non positioned slide.

The previous and next rules for the CSS transformed the elements by 100% in the relevant direction which would have appeared to work for one direction but not the other. By applying left:0 all the slides are in the middle but underneath the current active slide. Then the previous and next are moved off to each side so they can slide back in as required.

2 Likes
21

It’s not so bad. Here’s the code after being run through a linter:

Converting your var statements to be const was much easier than with the other code. A simple slide object contains all of the information that gets changed:

  const slide = {
    prev: 0,
    current: 0,
    next: 0,
    index: 0
  };

with all the code that references them being updated too, of which a part is:

    // slideCurrent = slideIndex;
    slide.current = slide.index;
    // slidePrev = slideIndex - 1;
    slide.prev = slide.index - 1;
    // slideNext = slideIndex + 1;
    slide.next = slide.index + 1;

Here’s a separate fork of the code after its been updated to use const:

On scanning further through the code, several of the comments didn’t seem to serve any great purpose.

    // set first slide as the active slide by default
    slides[0].classList.add("active");

Comments like that I have removed, and reduced others so that they just explain why something is being done.

In other cases I’ve updated the code so that the comment isn’t needed, such as with the touch support.

const touchsupport =
  "ontouchstart" in window ||
  navigator.maxTouchPoints > 0 ||
  navigator.msMaxTouchPoints > 0;
if (touchsupport) {
  // browser supports touch
  document.documentElement.className += " has-touch";
}

I have instead renamed the variable so that it more clearly expresses what the comment was saying.

const browserSupportsTouch =
  "ontouchstart" in window ||
  navigator.maxTouchPoints > 0 ||
  navigator.msMaxTouchPoints > 0;
if (browserSupportsTouch) {
  document.documentElement.className += " has-touch";
}

In the init function though, I did move the section setting the last item offset to the end of the function, so that the other parts above it make better sense as a group.

Another comment section with the events helps to inspire another improvement:

  // add event handlers to buttons
  next.addEventListener("click", function nextSlide() {
    //...
  });
  prev.addEventListener("click", function prevSlide() {
    //...
  });

By extracting the nextSlide and prevSlide functions, we can group the event listeners together so that it’s much easier to tell that they are assigning events, without needing the comment to tell us.

  next.addEventListener("click", nextSlide);
  prev.addEventListener("click", prevSlide);

The comment about putting the code at the end of the body I’ve moved to the top of the code, as it’s much more likely that people will see it there, and the CSS custom variables I’ve grouped together under a separate comment for both of them.

   // CSS custom variables
  const cssSliderSpeedProperty = "--slider-speed";
  const cssSliderSpeedValue = "0.5s";

I also removed commented out code from the nextSlide event handler. That’s about all that I’ve tweaked though, the final results being found in the following forked codePen:

https://codepen.io/pmw57/pen/LYbWYwx?editors=0010

2 Likes