patternjavascriptMinor
Two filters for products on a page
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:
Please check out this demo.
$(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:
We can now clean up the inside of that function further. For example, some things could be cached, and the second
The next thing to consider is your usage of classes. Your
Your solution does not scale. Instead, let's think about a
The resulting HTML might look like:
The jQuery initialization must take care to install the handlers only in the elements of the current collection:
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.
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.