0
\$\begingroup\$

I have a HTML5 range slider. i.e. <input class="range-slider" type="range" id="points" name="points" min="0" max="220">

I am showing, hiding, and changing some content based on the selected range slide values.. with Vanilla JS, I have all my functionality logic done, but I hate how much code it is. Is there a better, leaner, more elegant way to do this? The below achieves everything I want it to, it just seems like way too much code.

export function donationSlider() {
  resetAll()

  const slider = document.querySelector('.range-slider');
  const initial = document.querySelector('.tog-box-5');
  const defimg = document.querySelector('#level-img-6');
  const voluntBtn = document.querySelector('#volunteer-btn');
  const donateBtn = document.querySelector('#donate-btn');

  if (!slider || !initial || !defimg || !voluntBtn || !donateBtn)  return
  
  initial.classList.remove("hide");
  initial.classList.add("show");
  defimg.classList.add("show");

  slider.oninput = ({ target: { value } }) => {

    if (+value <= 1) {
      resetAll()
      document.querySelector('.tog-box-0').classList.add("show");
      document.querySelector('#level-img-1').classList.add("show");

      voluntBtn.href="?0"; 
      donateBtn.href="xyz.php";

    } else if (+value <= 10) { 
      resetAll()
      document.querySelector('.tog-box-1').classList.add("show");
      document.querySelector('#level-img-2').classList.add("show");

      voluntBtn.href="?10"; 
      donateBtn.href="xyz.php";

    } else if (+value <= 20) {
      resetAll()
      document.querySelector('.tog-box-2').classList.add("show");
      document.querySelector('#level-img-3').classList.add("show");

      voluntBtn.href="?20"; 
      donateBtn.href="xyz.php";

    } else if (+value <= 35) {
      resetAll()
      document.querySelector('.tog-box-3').classList.add("show");
      document.querySelector('#level-img-4').classList.add("show");

      voluntBtn.href="?35"; 
      donateBtn.href="xyz.php";

    } else if (+value <= 50) {
      resetAll()
      document.querySelector('.tog-box-4').classList.add("show");
      document.querySelector('#level-img-5').classList.add("show");

      voluntBtn.href="?50"; 
      donateBtn.href="xyz.php";

    } else if (+value <= 100) {
      resetAll()
      initial.classList.add("show");
      defimg.classList.add("show");

      voluntBtn.href="?100"; 
      donateBtn.href="xyz.php";

    } else if (+value <= 200) {
      resetAll()
      document.querySelector('.tog-box-6').classList.add("show");
      document.querySelector('#level-img-7').classList.add("show");

      voluntBtn.href="?200"; 
      donateBtn.href="xyz.php";

    } else if (+value <= 1600) {
      resetAll()
      document.querySelector('.tog-box-7').classList.add("show");
      document.querySelector('#level-img-8').classList.add("show");

      voluntBtn.href="?1600"; 
      donateBtn.href="xyz.php";
    } 
  }
  function resetAll() { 
    let togs = document.querySelectorAll('[class*="tog-box"]');
    for (var i = 0; i < togs.length; ++i) {
      togs[i].classList.add('hide');
      togs[i].classList.remove('show');
    }
    let imgz = document.querySelectorAll('.level-imgs');
    for (var i = 0; i < imgz.length; ++i) {
      imgz[i].classList.add('hide');
      imgz[i].classList.remove('show');
    }
  }
}
\$\endgroup\$
1
  • \$\begingroup\$ Just a quick note: unless you have 3 states for the elements, you can just have the .hide class, and when you want to show it, you could just remove it, instead of having an additional .show class. \$\endgroup\$
    – morbusg
    Jun 30, 2022 at 18:22

1 Answer 1

1
\$\begingroup\$

Without a minimum working example, I could not test that this code actually works- or whether my changes to it work too. The following advice should be applicable regardless- but it might not work as a complete drop-in.

The very first thing that I had to sort out was the indentation... 3 spaces?? To each their own, I guess :')

Now that the altruistic part is out of the way, there's three main points of contention I have with your code.

  1. There's a lot of repetitive and redundant code. This can be problematic to maintain and understand.
  2. It might be worth rounding the slider values to the nearest 10. This way, you could replace this ugly if-else block with a nice switch-case. I've not done this in my refactor of this code, but you could certainly do this if you so choose.
  3. Error handling. I noticed that you just return if any of the consts are undefined.. Might make more sense to throw an Error here, and handle it properly further up the stack.

I managed to get your 95 lines down to 51 lines without comments and newlines, 70 without comments, 81 with comments and newlines.

Some of the names of the functions I wrote are a little.. lacking. I'd pick better names, if I had the time :)

Without further ado:

export function donationSlider() {
    resetAll()

    const slider = document.querySelector('.range-slider');
    const initial = document.querySelector('.tog-box-5');
    const defimg = document.querySelector('#level-img-6');
    const voluntBtn = document.querySelector('#volunteer-btn');
    const donateBtn = document.querySelector('#donate-btn');

    if (!slider || !initial || !defimg || !voluntBtn || !donateBtn)  return

    initial.classList.remove("hide");
    initial.classList.add("show");
    defimg.classList.add("show");

    const setDonationProperties = partial(setVolumeAndDonate, voluntBtn, donateBtn);
    const toggleFn = partial(toggleSet, setDonationProperties);

    slider.oninput = ({ target: { value } }) => {
        if (+value <= 1) {
            toggleFn(0, ["?0", "xyz.php"]);
        } else if (+value <= 10) {
            toggleFn(1, ["?10", "xyz.php"]);
        } else if (+value <= 20) {
            toggleFn(2, ["?20", "xyz.php"]);
        } else if (+value <= 35) {
            toggleFn(3, ["?35", "xyz.php"]);
        } else if (+value <= 50) {
            toggleFn(4, ["?50", "xyz.php"]);
        } else if (+value <= 100) {
            resetAll()
            initial.classList.add("show");
            defimg.classList.add("show");
            setDonationProperties("?100", "xyz.php");
        } else if (+value <= 200) {
            toggleFn(6, ["?200", "xyz.php"]);
        } else if (+value <= 1600) {
            toggleFn(7, ["?1600", "xyz.php"]);
        }
    }
}

// Used to partially apply arguments to a function
const partial = (fn, firstArg) => { return (...lastArgs) => { return fn(firstArg, ...lastArgs); } };

function reset(el) {
    el.classList.add('hide');
    el.classList.remove('show');
}

function resetAll() {
    document.querySelectorAll('[class*="tog-box"]').forEach(tog => reset(tog));
    document.querySelectorAll('.level-imgs').forEach(img => reset(img));
}

// Since tog-box-x is exactly 1 smaller than tog-box-y, it's easy to factor
// this out into a function.
function toggleBox(t) {
    resetAll();
    document.querySelector(`.tog-box-${t}`).classList.add("show");
    document.querySelector(`#level-img-${t+1}`).classList.add("show");
}

// This function takes in two references to a volume button and a donate button
// alongside values to update each of these two things by. Some bounds checks
// should probably be done here, i.e. val == null etc
function setVolumeAndDonate(vBtnRef, dBtnRef, vBtnVal, dBtnVal) {
    vBtnRef.href = vBtnVal;
    dBtnRef.href = dBtnVal;
}

// Could probably be a better name...
// If the php script name is the same each time (xyz.php)
// then the interface for this function could be cleaned up, by removing the
// second value of the array (toSet[1]) and replacing it with the actual script
// name.
function toggleSet(setDonationFn, toToggle, toSet) {
    toggleBox(toToggle);
    setDonationFn(toSet[0], toSet[1]);
}
```
\$\endgroup\$

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.