current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have a click event that calls a function (below, txtResize) which makes some CSS changes to html text. (Acts on Text of 3 unique elements).

Q: What is the best way to refactor this code to limit excessive if/else usage, AND restore the original state of ALL text elements once the user clicks away from this section?

Here is the partially working, ugly function: 

 function txtResize() {

        var clicked = this.id;

        if (clicked == "Text1")
        {
            $("#" + clicked).css("opacity", "1");
            $("#Text2, #Text3").css("opacity", "0.2");
        }
        else if (clicked == "Text2") 
        {
            $("#" + clicked).css("opacity", "1");
            $("#Text1, #Text3").css("opacity", "0.2");
        }
        else if (clicked == "Text3")
        {
            $("#" + clicked).css("opacity", "1");
            $("#Text1, #Text2").css("opacity", "0.2");
        }
         // code above is ok, below does not work

        else if ( !(clicked == "Text1") && !(clicked == "Text2") && !(clicked == "Text3") ) 
        {
            $("#Text1, #Text2, #Text3").css("opacity", "1");  // return to default state
        }   

 };
share|improve this question
add comment

1 Answer

up vote 1 down vote

This is actually a pretty classic problem and I've addressed it in my code on numerous occasions. One of the simplest ways to do what you're doing is to just reset the state for all the elements and then correct it.

function txtResize() {
    var clicked = this.id;

    if ( clicked === "Text1" || clicked === "Text2" || clicked === "Text3" ) { //one of our text's were clicked
        $("#Text1, #Text2, #Text3").css("opacity", "0.2"); //reset them all to .2 opacity
        $(this).css("opacity", "1");//set this elements opacity to full
    } else { //its clearly not Text1/2/3
        $("#Text1, #Text2, #Text3").css("opacity", "1");  // return to default state
    }
 };

One nit-picky point is that we tend to prefer lower case in attributes for html. I would recommend text1, text2... or even better would be to give an id that better described what the element is on the page.

share|improve this answer
    
Thanks mega, couple issues: using $(this) doesn't work for me, but replace it with $("#" + clicked) will. Also, the opacity does not automatically restore itself, I still need to explicitly call another function to get the opacity to return. –  DeBraid Jan 20 at 21:35
    
I'd be interested in seeing an example where $(this) doesn't wrap the same element as $("#" + this.id). Do you have multiple elements with the same id on your page (quite a poor practice)? If you'd set up a jsfiddle or jsbin I'd play around with the code some more –  megawac Jan 21 at 0:20
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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