7
\$\begingroup\$

How can I refactor this code? I know it's repetitive but I'm not sure how to fix it.

$(function() {
    $('#category-1-button a').bind('click', function() {
        $(this).css({opacity:'1'});
        $('#category-2-button a,#category-3-button a').css({opacity:'0.4'});
        $('#blog-headers').css({backgroundPosition: '0 0'});
        $('#category_2,#category_3').hide(0, function() {
            $('#category_1').show(0);
        });
    });
});

$(function() {
    $('#category-2-button a').bind('click', function() {
        $(this).css({opacity:'1'});
        $('#category-1-button a,#category-3-button a').css({opacity:'0.4'});
        $('#blog-headers').css({backgroundPosition: '0 -144px'});
        $('#category_1,#category_3').hide(0, function() {
            $('#category_2').show(0);
        });
    });
});

$(function() {
    $('#category-3-button a').bind('click', function() {
        $(this).css({opacity:'1'});
        $('#category-1-button a,#category-2-button a').css({opacity:'0.4'});
        $('#blog-headers').css({backgroundPosition: '0 -288px'});
        $('#category_1,#category_2').hide(0, function() {
            $('#category_3').show(0);
        });
    });
});
\$\endgroup\$
0

4 Answers 4

1
\$\begingroup\$

The following will work right away. I did the following:

  1. Changed your bind() to a click(), which is more precise.
  2. Always set the opacity of all relevant links except this to 0 and then set the current one to 0.4.
  3. Combined all functions by using an if statement that compares parents. There is a slight performance loss here compared to three different functions.

Code:

$(function() {
    $('#category-1-button a, #category-2-button a, #category-3-button a').click(function () {
        $('#category-1-button a,#category-2-button a,#category-3-button a').not(this).css({opacity:'0.4'});
        $(this).css({opacity:'1'});
        var myParent = $(this).parent();
        if (myParent == $('#category-1-button')) {
            $('#blog-headers').css({backgroundPosition: '0 0'});
            $('#category_3,#category_2').hide(0, function() {
                $('#category_1').show(0);
            });
        } else if (myParent == $('#category-2-button')) {
            $('#blog-headers').css({backgroundPosition: '0 -144px'});
            $('#category_1,#category_3').hide(0, function() {
                $('#category_2').show(0);
            });
        } else if (myParent == $('#category-3-button')) {
            $('#blog-headers').css({backgroundPosition: '0 -288px'});
            $('#category_1,#category_2').hide(0, function() {
                $('#category_3').show(0);
            });
        }
    });
}

NOTE: I borrowed the .not(this) from patrick dw's comment.

\$\endgroup\$
3
  • \$\begingroup\$ I think you put the .not(this) in the wrong spot. I imagine you meant to place it before the .css() call instead of before .click(). Thanks for the credit though. :o) \$\endgroup\$
    – patrick dw
    Jun 10, 2011 at 2:22
  • \$\begingroup\$ @patrick, I updated that probably less than a minute before your comment. Thanks, though! \$\endgroup\$
    – Justin Satyr
    Jun 10, 2011 at 2:26
  • \$\begingroup\$ @Justin: Ah, I must have been looking at a stale version. \$\endgroup\$
    – patrick dw
    Jun 10, 2011 at 2:29
14
\$\begingroup\$

Give each #category-n-button a class like category_button.

Bind the handler in the each()[docs] method so that you can use the index argument to calculate the background position.

Use this to reference the element that received the event in the click handler.

Use the not()[docs] method to exclude the this element from the other category buttons when setting the opacity.

Use the filter()[docs] method to show the category element that pertains to the index of .each() + 1.

$(function() {
    var categories = $('[id^="category_"]');
    var category_buttons_a = $('.category_button a').each(function( idx ) {
        $(this).bind('click', function() {
            $(this).css({opacity:'1'});
            category_buttons_a.not(this).css({opacity:'0.4'});
            $('#blog-headers').css({backgroundPosition: '0 ' + (idx * -144) + 'px'});
            categories.hide().filter( '#category_' + (idx + 1) ).show();
        });
    });
});
\$\endgroup\$
7
  • 1
    \$\begingroup\$ This is clearly the better answer. I don't know if your use of partial matching in the id selector reduces performance, but my answer's if statements and node comparisons definitely do. \$\endgroup\$
    – Justin Satyr
    Jun 10, 2011 at 2:25
  • \$\begingroup\$ @Justin: Yes, in browsers that don't support querySelectorAll, that initial partial ID selection will be slow, but since it only occurs once, then is cached, the overall impact is small. \$\endgroup\$
    – patrick dw
    Jun 10, 2011 at 2:26
  • \$\begingroup\$ ...I really should have cached the $('#blog-headers') selection as well. I hate re-running selectors. \$\endgroup\$
    – patrick dw
    Jun 10, 2011 at 2:27
  • \$\begingroup\$ Really nice answer, and nice tips (especially the not and filter). Thanks for sharing! Hey! This could be a recruiting test for my company! ;) \$\endgroup\$ Jun 10, 2011 at 5:01
  • \$\begingroup\$ You can improve performance a little by prefixing the element name to your $('.category_button a') selector. In those circumstances Sizzle will use getElementsByTagName(...) to filter before attempting a class match, instead of having to check each DOM node specifically. You should also do this for your attribute selector $('[id^="category_"]') too, and if at all possible, pass in a context (a point from which Sizzle will search). \$\endgroup\$ Jun 11, 2011 at 9:50
1
\$\begingroup\$

I believe I've covered all the bases with this one.

(function($){
"use strict";
var $categoryButtonLinks;

$categoryButtonLinks = $('#category-1-button a, #category-2-button a, #category-3-button a').click(clickCategoryButtonLink);

$categories = $('#category_1, #category_2, #category_3');

function clickCategoryButtonLink(e)
{
  var $this, $category, index, offset;
  $this = $(this).css('opacity', '1');
  index = $categoryButtonLinks.index($this);
  offset = -144 * index;
  index += 1;
  $category = $('#category_'+index);
  $categoryButtonLinks.not($this).css('opacity', '0.4');
  $('#blog-headers').css('background-position', '0 ' + offset + 'px' );
  $categories.not($category).hide(0, function(){
    $category.show(0);
  });
}

})(jQuery);
\$\endgroup\$
1
\$\begingroup\$

patrick dw's answer is great (+1), but I'd like to suggest an alternative, namely refactoring all the dynamic styling out into the style sheet. While it does make te style sheet longer, especially due to the "repeated" selectors, it does have the (IMHO very important) advantage to put all the styles where they belong.

Assuming the category buttons look like this:

<div id="class="category-1-button" class="category_button">
  <a href="#Category_1">Category 1</a>
</div>

Notice the reference to the category in the href. If done right, this also can have the advantage, that the links would work without JavaScript (not that anyone cares about that nowadays...)

And give the categories a common class, too (e.g. "category").

CSS

.category_button a {
  opacity: 0.4;
}

.category {
   display: none;
 }

/* The following rules/selectors could/should be generated by a script */
.category_1_selected #category-1-button a,
.category_2_selected #category-2-button a,
.category_3_selected #category-3-button a {
  opacity: 1;
}

.category_1_selected #category_1,
.category_2_selected #category_2,
.category_3_selected #category_3 {
  display: block;
}

.category_1_selected #blog-headers {
   background-position: 0 0;
 }

.category_2_selected #blog-headers {
   background-position: 0 -144px;
 }

.category_3_selected #blog-headers {
   background-position: 0 -288px;
 }

JavaScript

$(function() {
  var category_buttons_a = $('.category_button a').click(function() {
    // I'm putting the class on the body as an example, but any other element the
    // surrounds the buttons, categorys and the blog header is fine.
    $("body").removeClass().addClass(this.href.slice(1) + "_selected");
  });
});

One remark on the HTML of the links: I based it on how I assume your links look like and IMHO it's wrong to do it like that. Instead it would be better to put the class (and if necessary the id) directly on the link and not on it's parent element:

<div>
  <a href="#Category_1" id="class="category-1-button" class="category_button">Category 1</a>
</div>
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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