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

Two filters for products on a page

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

Problem

I'm using two category filters from this demo. I give the filters two different sets of classes so they won't affect each other. Since I'm very new to jQuery, all I can come up with is repeating the same code with a different set of classes. I wonder if there's a shorter way to write this:

$(function(){

    /* filter1 */
    $(".category-menu ul li").click(function(){
        var CategoryID = $(this).data('category');
        $('.category-menu ul li').removeClass('cat-active');
        $(this).addClass('cat-active');

        $('.prod-cnt').each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 

    });

    $('.category-menu select').change(function(){
        var CategoryID = $(this).find('option:selected').data('category');
        $('.prod-cnt').each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });

    /* filter2 */
    $(".category-menu2 ul li").click(function(){
        var CategoryID = $(this).data('category');
        $('.category-menu2 ul li').removeClass('cat-active2');
        $(this).addClass('cat-active2');

        $('.prod-cnt2').each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 

    });

    $('.category-menu2 select').change(function(){
        var CategoryID = $(this).find('option:selected').data('category');
        $('.prod-cnt2').each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });

});


Please check out this demo.

Solution

When you see that you're basically copy-pasting the code and changing a few values, you can usually encapsulate all the common stuff inside a function. Here:

var registerStuff = function(category, active, prod, select) {
    $(category).click(function(){
        var CategoryID = $(this).data('category');
        $(category).removeClass(active);
        $(this).addClass(active);

        $(prod).each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });

    $(select).change(function(){
        var CategoryID = $(this).find('option:selected').data('category');
        $(prod).each(function(){
            if(($(this).hasClass(CategoryID)) == false){
                $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });
};

registerStuff(".category-menu  ul li", 'cat-active',  '.prod-cnt',  '.category-menu  select');
registerStuff(".category-menu2 ul li", 'cat-active2', '.prod-cnt2', '.category-menu2 select');


We can now clean up the inside of that function further. For example, some things could be cached, and the second $(category) is uneccessary. The == false is a code smell, rather use a negation. The jQuery hide() method is better than spelling out the css(). Blending out the prods of the wrong category is common code, and can be factored out as well.

var registerStuff = function(category, active, prod, select) {
    var displayOnlyWantedCategory = function($prods, categoryId) {
        $prods.each(function() {
            var $this = $(this);
            if (! $this.hasClass(categoryId)) {
                $this.hide();
            }
        });
        $('.' + categoryId).fadeIn(); 
    };

    var $category = $(category);
    $category.click(function() {
        var $this = $(this);
        $category.removeClass(active);
        $this.addClass(active);

        displayOnlyWantedCategory($(prod), $this.data('category'));
    });

    $(select).change(function() {
        displayOnlyWantedCategory($(prod), $(this).find('option:selected').data('category'));
    });
};


The next thing to consider is your usage of classes. Your category-menu and category-menu2 are visually and functionally identical, they're only two instances of the same idea. What happens when you want to have fifteen category-menus on the same page? I wouldn't want to see the CSS for that:

.category-menu,
 .category-menu2,
 .category-menu3,
   ...
 .category-menu14,
 .category-menu15 {
   ...
 }


Your solution does not scale. Instead, let's think about a product-collection element, which contains a few buttons to select a category, and a few items which can be displayed. We want to limit the effect of selecting a category to the current collection only.

The resulting HTML might look like:


  
    All
    Category 1
    Category 2
  
  
    Item 1 [cat 1]
    Item 2 [cat 2]
    Item 3 [cat 1]
  


The jQuery initialization must take care to install the handlers only in the elements of the current collection:

$(function() {
    $('.product-collection').each(function() {
        var $collection = $(this);
        var $selectors = $collection.find('.product-collection__selector');
        var $items     = $collection.find('.product-collection__item');

        $selectors.click(function() {
            var $selector = $(this);
            var cat = $selector.data('product-collection__category');

            $selectors.removeClass('product-collection__selector--active');
            $selector.addClass('product-collection__selector--active');

            if (cat) {
                $items.each(function() {
                    var $item = $(this);
                    if ($item.data('product-collection__category') == cat)
                        $item.fadeIn();
                    else
                        $item.hide();
                });
            }
            else {
                $items.fadeIn();
            }
        });
    });
});


See it in action here: http://jsfiddle.net/Qv6QE/3/

My class names may seem excessively verbose, but they avoid namespace conflicts by following the Block-Element-Modifier naming scheme.

Code Snippets

var registerStuff = function(category, active, prod, select) {
    $(category).click(function(){
        var CategoryID = $(this).data('category');
        $(category).removeClass(active);
        $(this).addClass(active);

        $(prod).each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });

    $(select).change(function(){
        var CategoryID = $(this).find('option:selected').data('category');
        $(prod).each(function(){
            if(($(this).hasClass(CategoryID)) == false){
                $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });
};

registerStuff(".category-menu  ul li", 'cat-active',  '.prod-cnt',  '.category-menu  select');
registerStuff(".category-menu2 ul li", 'cat-active2', '.prod-cnt2', '.category-menu2 select');
var registerStuff = function(category, active, prod, select) {
    var displayOnlyWantedCategory = function($prods, categoryId) {
        $prods.each(function() {
            var $this = $(this);
            if (! $this.hasClass(categoryId)) {
                $this.hide();
            }
        });
        $('.' + categoryId).fadeIn(); 
    };

    var $category = $(category);
    $category.click(function() {
        var $this = $(this);
        $category.removeClass(active);
        $this.addClass(active);

        displayOnlyWantedCategory($(prod), $this.data('category'));
    });

    $(select).change(function() {
        displayOnlyWantedCategory($(prod), $(this).find('option:selected').data('category'));
    });
};
.category-menu,
 .category-menu2,
 .category-menu3,
   ...
 .category-menu14,
 .category-menu15 {
   ...
 }
<div class="product-collection">
  <ul>
    <li class="product-collection__selector
               product-collection__selector--active"
        data-product-collection__category="">All</li>
    <li class="product-collection__selector"
        data-product-collection__category="cat1">Category 1</li>
    <li class="product-collection__selector"
        data-product-collection__category="cat2">Category 2</li>
  </ul>
  <ul>
    <li class="product-collection__item"
        data-product-collection__category="cat1">Item 1 [cat 1]</li>
    <li class="product-collection__item"
        data-product-collection__category="cat2">Item 2 [cat 2]</li>
    <li class="product-collection__item"
        data-product-collection__category="cat1">Item 3 [cat 1]</li>
  </ul>
</div>
$(function() {
    $('.product-collection').each(function() {
        var $collection = $(this);
        var $selectors = $collection.find('.product-collection__selector');
        var $items     = $collection.find('.product-collection__item');

        $selectors.click(function() {
            var $selector = $(this);
            var cat = $selector.data('product-collection__category');

            $selectors.removeClass('product-collection__selector--active');
            $selector.addClass('product-collection__selector--active');

            if (cat) {
                $items.each(function() {
                    var $item = $(this);
                    if ($item.data('product-collection__category') == cat)
                        $item.fadeIn();
                    else
                        $item.hide();
                });
            }
            else {
                $items.fadeIn();
            }
        });
    });
});

Context

StackExchange Code Review Q#41544, answer score: 8

Revisions (0)

No revisions yet.