HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavascriptMinor

Filter by vendor-feature with jQuery & Isotope

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withvendorisotopefilterjqueryfeature

Problem

Task description:
A set of product-thumbnails is shown. The user can filter the shown products by vendor(s). Done by clicking one or multiple buttons.

I used jQuery together with the Isotope-plugin: http://isotope.metafizzy.co/

First I had the idea of using iteration for assembling the filter-criterium string:



$(function() {
$products = $('.products');

$products.isotope({
itemSelector: '.product',
layoutMode: 'fitRows'
})

$('.vendor').click(function() {
let filterCrit = '';

$(this).toggleClass('active');
// Iterate over the vendor-buttons ...
$('.vendor').each(function(i, product) {
// ... if button is "active" => Add vendor-name to filter-criterium.
if ($(this).hasClass('active')) {
filterCrit += ', .' + $(this).data('vendor');
}
});
// Remove the trailing ", " and use the string as filter-criterium.
$products.isotope({
filter: filterCrit.slice(2)
});
});
});

* {
padding: 0;
margin: 0;
}
body {
background-color: #282828;
color: #efefef;
}
.products {
width: 100%;
}
.product {
margin-left: 5px;
margin-bottom: 5px;
float: left;
width: 100px;
}
.wrap {
max-width: 1300px;
width: 100%;
margin: 30px auto;
padding: 10px 30px;
}
nav {
padding: 10px 20px;
overflow: hidden;
display: flex;
justify-content: space-between;
flex-wrap: wrap;
width: 100%;
}
a.vendor {
text-decoration: none;
text-transform: uppercase;
font-size: 1.4rem;
padding: 20px 10px;
border: 3px solid white;
display: inline-block;
width: 20%;
min-width: 125px;
text-align: center;
color: #efefef;
margin-bottom: 5px;
}
a.vendor:hover {
color: #101010;
background-color: #d7d7d7;
}
a.vendor.active {
background-color: silver;
color: #010101;
box-shadow: inset 1px 1px 2px #555;
}

`

Apple
Garmin
Fitbit
Polar






















Solution

Dropping variables in the global scope

$products is not declared. You are dropping it into the global scope... Consider using strict mode ("use strict";) to cause a SyntaxError rather than allowing this to happen.
Creating a string with a common delimiter

In the first snippet you use a somewhat ugly way of creating a string with a common delimiter. It is not very readable, and produces that "trailing comma" that you later have to remove. Instead, use Array.prototype.join():

$('.vendor').click(function() {
  let filterVendors = [];

  $(this).toggleClass('active');
  // Iterate over the vendor-buttons ...
  $('.vendor').each(function(i, product) {
    // ... if button is "active" => Add vendor-name to filter-criterium.
    if ($(this).hasClass('active')) {
      filterVendors.push('.' + $(this).data('vendor'));
    }
  });

  $products.isotope({
    filter: filterVendors.join(', ')
  });
});


Note: If you didn't need the string, but instead could use a jQuery collection of elements, you could use let a = $('') and a.add('another selector') to build the collection. This would be cleaner than dynamically creating one big selector, but is not possible in this specific case, because you need the selector string.
Semicolons

Both your first and second snippet are missing a semicolon. Don't rely on automatic semicolon insertion (ASI), as it will more likely bite you than save you.
Function with side-effect

In the second snippet, getCurrent(..) is a function with a side-effect. It both toggles the current selection and returns a selector. Try to avoid this.

I am unsure why you have a closure inside an anonymous function. Sure, it hides active from the rest of the function... but it requires you to have that function with side-effect. Just pull it out into the anonymous function, and make two functions.
.attr(data- instead of .data(

In the second snippet, use .data(..) instead of .attr(..) to benefit from (future) optimizations related to data storage on DOM elements.
let vs var

You are using var in the second snippet. While it is not wrong to use var, be consistent. Decide for which version of Ecmascript you are writing, and which browsers need to be supported, and use that version consistently. In this case, use let for both of those variables.
Arrow functions

Arrow functions are not supported in Internet Explorer. If you are using something like babel to convert newer syntax to older syntax that is more widely supported you have nothing to worry about.

In the end, I would suggest to use the first version, as it is clearer. The second version might give a better performance if you have an incredible large number of categories, but your application will be unusable from a consumer standpoint if that is the case. In any case, the number of product elements selected will always overshadow the number of categories selected.

$(function() {
  let $products = $('.products');

  $products.isotope({
    itemSelector: '.product',
    layoutMode: 'fitRows'
  });

  $('.vendor').click(function() {
    let filterVendors = [];

    $(this).toggleClass('active');
    // Iterate over the vendor-buttons ...
    $('.vendor').each(function(i, product) {
      // ... if button is "active" => Add vendor-name to filter-criterium.
      if ($(this).hasClass('active')) {
        filterVendors.push($(this).data('vendor'));
      }
    });
    // Remove the trailing ", " and use the string as filter-criterium. 
    $products.isotope({
      filter: filterVendors.join(', ')
    });
  });
});

Code Snippets

$('.vendor').click(function() {
  let filterVendors = [];

  $(this).toggleClass('active');
  // Iterate over the vendor-buttons ...
  $('.vendor').each(function(i, product) {
    // ... if button is "active" => Add vendor-name to filter-criterium.
    if ($(this).hasClass('active')) {
      filterVendors.push('.' + $(this).data('vendor'));
    }
  });

  $products.isotope({
    filter: filterVendors.join(', ')
  });
});
$(function() {
  let $products = $('.products');

  $products.isotope({
    itemSelector: '.product',
    layoutMode: 'fitRows'
  });

  $('.vendor').click(function() {
    let filterVendors = [];

    $(this).toggleClass('active');
    // Iterate over the vendor-buttons ...
    $('.vendor').each(function(i, product) {
      // ... if button is "active" => Add vendor-name to filter-criterium.
      if ($(this).hasClass('active')) {
        filterVendors.push($(this).data('vendor'));
      }
    });
    // Remove the trailing ", " and use the string as filter-criterium. 
    $products.isotope({
      filter: filterVendors.join(', ')
    });
  });
});

Context

StackExchange Code Review Q#139830, answer score: 2

Revisions (0)

No revisions yet.