Pure JS multi-slide carousel

Question

On our home page we need simple multi-slide carousel. It is our highest traffic page, so we decided to remove all libraries (including jQuery) in order to increase page speed.

Functionality we trying to achieve is similar to this

Here is the code we have now: Codepen

function getStyleValue(elem, property) {
  return window.getComputedStyle(elem).getPropertyValue(property);
}

const arrows = document.querySelectorAll(".carousel .arrow"),
  [leftArrow, rightArrow] = arrows,
  slideContainer = document.querySelector(".carousel .slide-container"),
  slides = document.getElementsByClassName("slide"),
  amt = parseInt(getStyleValue(slides[0], "width"));

try {
  let trans = parseInt(
    getStyleValue(slideContainer, "transform")
      .split(",")[4]
      .trim()
  );
} catch (e) {
  console.log("`trans` being set to 0");
  trans = 0;
}

function slide() {
  if (this === rightArrow) {
    trans -= amt;
  } else if (this === leftArrow) {
    trans += amt;
  }
  if (trans <= amt * slides.length / 2 * -1) {
    rightArrow.style.display = "none";
  } else if (trans >= 0) {
    leftArrow.style.display = "none";
  } else {
    arrows.forEach(arrow => (arrow.style.display = "inline-block"));
  }
  slideContainer.style.transform = `translate(${trans}px)`;
}

arrows.forEach(arrow => arrow.addEventListener("click", slide));

body *::selection {
  background: none;
}

.carousel {
  --height: 128px;
  --width: 120px;
  display: block;
  margin: 100px auto 0;
  height: var(--height);
  text-align: center;
}

.carousel * {
  display: inline-block;
  position: relative;
  vertical-align: top;
  height: 100%;
}

.arrow-container {
  --width-arrow: 64px;
  width: var(--width-arrow);
}

.arrow {
  --arrow-width: calc(var(--width-arrow) / 2);
  position: absolute;
  top: 50%;
  left: 50%;
  width: var(--arrow-width);
  height: var(--arrow-width);
  background-color: white;
  font-weight: bold;
  line-height: var(--arrow-width);
  border-radius: 50%;
  transform: translate(-50%) translateY(-50%);
  transition: background-color 111ms, color 222ms;
  cursor: pointer;
}

.arrow:hover {
  background-color: rgba(0, 0, 0, 0.2);
  color: white;
}

.arrow:active {
  transform: scale(0.9) translate(-50%) translateY(-50%);
}

.arrow.left {
  display: none;
}

.display-container {
  /* * 4 to show 4 slides */
  width: calc(var(--width) * 4 - 2px);
  overflow-x: hidden;
}

.slide-container {
  /* * 8 cause that's the number of slides */
  width: calc(var(--width) * 8);
  /* start by going left 2 slides */
  /* transform: translate(calc(var(--width) * -2)); */
  transition: transform 333ms;
}

.slide {
  margin-left: -4px;
  width: var(--width);
  background-color: salmon;
}

.slide:nth-child(2n) {
  background-color: cornflowerblue;
}

<div class="carousel">
  <div class="arrow-container">
    <span class="arrow left">&lt;</span>
  </div>
  <div class="display-container">
    <div class="slide-container">
      <div class="slide">1</div>
      <div class="slide">2</div>
      <div class="slide">3</div>
      <div class="slide">4</div>
      <div class="slide">5</div>
      <div class="slide">6</div>
      <div class="slide">7</div>
      <div class="slide">8</div>
    </div>
  </div>
  <div class="arrow-container">
    <span class="arrow right">&gt;</span>
  </div>
</div>

Answer 1

Minor issues

• Don't use window. inconsistently. You use it in some places and then not in others. It's just noise so the best option is that if you find yourself typing window.blah you have added 7 needless characters.

• Don't use compound declarations, they can be a source of hard to spot errors, especially as you are not using strict mode.

• "use strict" Never release code that does not use strict mode, and it must have used strict mode from day one.

• Avoid using parseInt unless you define the radix, eg parseInt("16",10). Better way to parse a string to a number is via Number(string). Or a safer form const number = isNaN(str) ? defaultNumber : Number(str);

• Your slide function at first tests for both the left and right arrow. but if the only events that will generate the call are the two arrows the second statement should be else and not include the redundant if statement.

• Don't do complex expressions inside conditional statements. Generally conditional statement should be limited to the test. You had if (trans <= amt * slides.length / 2 * -1) { would be better split as two lines; const pos =amt * slides.length / 2 * -1; then if(trans <= pos) {

Bad code.

There are some that will not agree, this is an opinion on good coding practice for JavaScript.

The code in question

try {
  let trans = parseInt(
    getStyleValue(slideContainer, "transform")
      .split(",")[4]
      .trim()
  );
} catch (e) {
  console.log("`trans` being set to 0");
  trans = 0;
}

There are very few situations in javascript when you need to use a try catch block. I feel that if you are using them you are being lazy.

There are many problems that can be overlooked when using try catch. Here are just 3 for your case.

1. You catch all errors, not just the one you have planned for. Code running in the catch should always check the error and only handle the one you intend to handle, all other errors should be thrown to a higher level.

   As you have it you will consume all errors, errors that could cause problems later in your code but will not know about till way after it first threw an error.

2. You are forcing the error via the trim function TypeError: Cannot read property 'trim' of undefined this is no guarantee of the problem being caught.

   I was at first puzzled as to why the trim was there as parseInt will do it for you, till it dawned on me, to throw the (incorrect) error.

   So not only badly using the try catch you are not using a standard method to throw a known error. You should have used throw and thrown a new Error("blah blah"); (I put "blah blah" because I do not consider it an error at all and have no words to describe it.)

3. Major scope problem You will never get the correct result as trans is declared as scoped to the try block and the catch if invoked assigns to a new global scope trans.

   The only value trans will have after that code has run is as an undefined undeclared variable (Very dangerous) or as the global scoped trans = 0;

It does not take much effort to avoid the try catch, and in this case the cluster of problems it has set up in your code.

Quick rewrite.

I did not look at variable naming, function design and layout, and other issues as It was the try catch the got my attention.

Thus all I did was rewrite the try catch block as a function returning transform property or zero. If you run it here as snippet, it will throw for a very different reason though.

function getTrans(element){
  var value = getStyleValue(element, "transform");
  value = value.split(",")[4];
  if(value !== undefined){
    return Number(value);
  }
  return 0;
}
var trans = getTrans(slideContainer);