Need Help Optimizing Vanilla JS Function

Question

I'm starting to use more Vanilla JS and rewriting old code that was in jQuery into Vanilla JS.

What I'm trying to do:

So, I have a list of filters. Under that, I have a list of stories under those filters. The idea is that when you click on a filter, you will only see the stories that match. If you click the "All" filter, you will see all the stories.

JQuery Solution

With jQuery, this is the old code:

var $filters = $('.filters').click(function(e) {
  e.preventDefault();
  if ($(this).data('filter') == 'all') {
    $('.directory .item').fadeIn(450);
  } else {
    var $el = $('.' + $(this).data('filter')).fadeIn(450);
    $('.directory .item').not($el).hide();
  }
  $filters.removeClass('selected');
  $(this).addClass('selected');
})

Vanilla JS Solution

I'm rebuilding this using Vanilla JS and have what I built over on CodePen: https://codepen.io/rmlumley/pen/ExjVxgw

You can also see the JS here:

// Find list of items with a class of filters
const filters = document.querySelectorAll('.filters');
function toggleFilter(e) {
    e.preventDefault(); // Prevent link from working.

    // Remove Selected class from other filters.
    for (var i = 0; i < filters.length; i++) {
        filters[i].classList.remove('selected');
    }
    // Add selected class to this filter.
    this.classList.add('selected');

    // Grab all Items.
    const all = document.querySelectorAll('.directory .item'); 
    // If selecting filter "All", then show all items.
    if (this.dataset.filter == 'all') {
        for (var i = 0; i < all.length; i++) {
            all[i].classList.remove('hide');
        }
    // Otherwise, filter by the data-attribute of filter that is set.
    } else {
        const filter = this.dataset.filter;
        // First off, hide all elements.
        for (var i = 0; i < all.length; i++) {
            all[i].classList.add('hide');
        }
        // Now show all elements that match.
        let selected = document.querySelectorAll(`.directory .${filter}`);
        for (var i = 0; i < selected.length; i++) {
            selected[i].classList.remove('hide');
        }
    }
}

// Event Listener on any Filter that is clicked.
filters.forEach(filter => filter.addEventListener('click', toggleFilter));

Questions

Is there a better way to write this function?
How do I animate it similar to the fadeIn I was using with jQuery?

volt · Accepted Answer

How do I animate it similar to the fadeIn I was using with jQuery?

Since you're adding the class hidden to non-active items, you can fade them in with a CSS animation of opacity like so:

.item {
  animation: fade 0.75s ease-in-out;
}

.hide {
  display: none;
}

@keyframes fade {
  from {
    opacity: 0;
  }
  to {
    opacity: 1;
  }
}

You can skip !important for .hidden It's not needed.

Is there a better way to write this function?

There's always a better way to write a piece of code and it can be very subjective. A few things I would recommend:

use arrow functions when you don't have a lot of different scopes
for loops are faster if you iterate over the same function 1000 times, but in the real world, forEach is a lot more readable.
Keep things that you don't need to constantly query outside of loops.
If there's code that doesn't need to be executed if a certain condition is met (example: user clicked all filter) use return;
Your code should explain itself and you shouldn't need to provide a comment for every line.
Consider reading about the spread syntax that can be used to convert nodeLists to arrays.
consider reading about const and let

That said, here's how I would write that JS

const directory = document.querySelector('.directory')
const allItems = [...directory.querySelectorAll('.item')];
const filterWrapper = document.getElementById('isotope-filters')

const toggleFilter = event => {
  if (!event.target.dataset.filter) return;
  event.preventDefault();

  const filter = event.target.dataset.filter;
  const oldSelected = filterWrapper.querySelector('.selected');
  const currentSlected = [...directory.querySelectorAll(`.${filter}`)];

  oldSelected.classList.remove('selected');
  event.target.classList.add('selected');

  if (filter === 'all') {
    allItems.forEach(item => {
      item.classList.remove('hide');
    })
    return
  }

  allItems.forEach(item => {
    item.classList.add('hide');
  })

  currentSlected.forEach(item => {
    item.classList.remove('hide');
  })
}

filterWrapper.addEventListener('click', toggleFilter)

and here's a working snippet of the entire thing

const directory = document.querySelector('.directory')
const allItems = [...directory.querySelectorAll('.item')];
const filterWrapper = document.getElementById('isotope-filters')

const toggleFilter = event => {
  if (!event.target.dataset.filter) return;
  event.preventDefault();

  const filter = event.target.dataset.filter;
  const oldSelected = filterWrapper.querySelector('.selected');
  const currentSlected = [...directory.querySelectorAll(`.${filter}`)];

  oldSelected.classList.remove('selected');
  event.target.classList.add('selected');

  if (filter === 'all') {
    allItems.forEach(item => {
      item.classList.remove('hide');
    })
    return
  }

  allItems.forEach(item => {
    item.classList.add('hide');
  })

  currentSlected.forEach(item => {
    item.classList.remove('hide');
  })
}

filterWrapper.addEventListener('click', toggleFilter)

.selected {
  border: 1px solid black;
  background-color: gray;
}

.item {
  border: 1px solid black;
  margin: 1em;
  padding: 1em;
  animation: fade 0.75s ease-in-out;
}

.hide {
  display: none;
}

@keyframes fade {
  from {
    opacity: 0;
  }
  to {
    opacity: 1;
  }
}


  All
  Bioethics
  Medical Engineering
  Metabolism
  Outreach
  Regenerative Biology
  Virology



  
    Story 1
    Story 2
    Story 3
    Story 4
    Story 5
    Story 6
    Story 7
    Story 8
    Story 9
    Story 10
    Story 11
    Story 12

Need Help Optimizing Vanilla JS Function

What I'm trying to do:

JQuery Solution

Vanilla JS Solution

Questions

Answers (2)

Related Questions