3
\$\begingroup\$

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;
}
<div class="wrap">
  <nav>
    <a href="#" data-vendor="apple" class="vendor apple">Apple</a>
    <a href="#" data-vendor="garmin" class="vendor garmin">Garmin</a>
    <a href="#" data-vendor="fitbit" class="vendor fitbit">Fitbit</a>
    <a href="#" data-vendor="polar" class="vendor polar">Polar</a>
  </nav>
  <div class="products">
    <div class="product apple">
      <img src="http://placehold.it/100x100/f00" alt="apple-product" />
    </div>
    <div class="product garmin">
      <img src="http://placehold.it/100x100/0f0" alt="garmin-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product garmin">
      <img src="http://placehold.it/100x100/0f0" alt="garmin-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product polar">
      <img src="http://placehold.it/100x100/00f" alt="polar-product" />
    </div>
    <div class="product polar">
      <img src="http://placehold.it/100x100/00f" alt="polar-product" />
    </div>
    <div class="product apple">
      <img src="http://placehold.it/100x100/f00" alt="apple-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product garmin">
      <img src="http://placehold.it/100x100/0f0" alt="garmin-product" />
    </div>
  </div>
</div>
<script src="https://npmcdn.com/[email protected]/dist/isotope.pkgd.min.js"></script>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

Working demo on CodePen: http://codepen.io/mizech/pen/bZXqwa

Then I had the idea of using a array for storing the active vendors:

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

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

  var getCurrent = (function() {
    var active = []; // Vendors currently active.

    return (vendor) => {
      let i = active.indexOf(vendor);
      // Update the directory.
      i > -1 ? active.splice(i, 1) : active.push(vendor);

      return active.join(', ');
    }
  })();

  $('.vendor').click(function() {
    $products.isotope({
      filter: getCurrent($(this).attr('data-vendor'))
    });

    $(this).toggleClass('active');
  });
});
* {
  padding: 0;
  margin: 0;
}
body {
  background-color: #282828;
  color: #efefef;
  font-family: Futura, "Trebuchet MS", Arial, sans-serif;
}
.products {
  width: 100%;
}
.product {
  margin-left: 5px;
  margin-bottom: 5px;
  float: left;
  width: 100px;
}
.wrap {
  max-width: 1300px;
  width: 100%;
  margin: 20px auto;
  padding: 10px 30px;
}
nav {
  padding: 10px 20px 25px;
  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: 15px 10px;
  border: 3px solid white;
  display: inline-block;
  width: 20%;
  min-width: 100px;
  text-align: center;
  color: #efefef;
}
a.vendor:hover {
  color: #101010;
  background-color: #d7d7d7;
}
a.vendor.active {
  background-color: silver;
  color: #010101;
  box-shadow: inset 1px 1px 2px #555;
}
<script src="https://npmcdn.com/[email protected]/dist/isotope.pkgd.min.js"></script>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div class="wrap">
  <nav>
    <a href="#" data-vendor=".apple" class="vendor apple">Apple</a>
    <a href="#" data-vendor=".garmin" class="vendor garmin">Garmin</a>
    <a href="#" data-vendor=".fitbit" class="vendor fitbit">Fitbit</a>
    <a href="#" data-vendor=".polar" class="vendor polar">Polar</a>
  </nav>
  <div class="products">
    <div class="product apple">
      <img src="http://placehold.it/100x100/f00" alt="apple-product" />
    </div>
    <div class="product garmin">
      <img src="http://placehold.it/100x100/0f0" alt="garmin-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product garmin">
      <img src="http://placehold.it/100x100/0f0" alt="garmin-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product polar">
      <img src="http://placehold.it/100x100/00f" alt="polar-product" />
    </div>
    <div class="product polar">
      <img src="http://placehold.it/100x100/00f" alt="polar-product" />
    </div>
    <div class="product apple">
      <img src="http://placehold.it/100x100/f00" alt="apple-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product fitbit">
      <img src="http://placehold.it/100x100/0ff" alt="fitbit-product" />
    </div>
    <div class="product garmin">
      <img src="http://placehold.it/100x100/0f0" alt="garmin-product" />
    </div>
  </div>
</div>

Live-Demo on CodePen: http://codepen.io/mizech/pen/WxVpAr

Finished with that I had to admit that the version with the array isn't actually less code.

I personally still prefer the second approach with the array.

Would like to read your opinions!

Any hints how the code could be improved would also be appreciated.

Improve this question
\$\endgroup\$
3
  • \$\begingroup\$ Instaead of $('.vendor').each(function(i, product) { and if ($(this).hasClass('active')) {, use $('.vendor.active').each( \$\endgroup\$
    – Tushar
    Aug 29, 2016 at 7:06
  • \$\begingroup\$ Check this code \$\endgroup\$
    – Tushar
    Aug 29, 2016 at 7:15
  • \$\begingroup\$ Awesome ! You're perfectly right: I should think first if an simple CSS-selector could do the job. Thanks a bunch. :) \$\endgroup\$
    – michael.zech
    Aug 29, 2016 at 7:16

1 Answer 1

Reset to default
2
\$\begingroup\$

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(', ')
    });
  });
});
Improve this answer
\$\endgroup\$
1

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.