If/else not working with click event in jQuery

Question

I'm a beginner in jQuery and trying to have a bit of fun. I'm trying to make a button that will change the background of the page when clicked, as well as change the text of the button. I only need two scenarios: - if the button says "1", change the button to "2" and the background to black; - if the button says "2", change the button to "1" and the background to white;

My code works for the first click but you can't click more than once.

What did I do wrong? Thanks for your time.

http://jsfiddle.net/6fRbM/7/

This is my HTML

<body>
<button id="lumiere">1</button>
</body>

This is my script

$(function() {
    $('#lumiere').bind('click', function(event) {
        if ($('#lumiere:contains("1")')) {
            $('body').css('background-color', "black")
            $('#lumiere').html('2')
        }
        else if ($('#lumiere:contains("2")')) {
            $('body').css('background-color', "white")
            $('#lumiere').html('1')
        }
    }
    );
}
);

nderscore · Accepted Answer · 2013-06-28 15:19:14Z

10

Your if statement will always return true, because even if your query returns no elements, it will still give you a jQuery object. Try checking the 'length' property instead:

$(function() {
    $('#lumiere').bind('click', function(event) {
        if ($('#lumiere:contains("1")').length) {
            $('body').css('background-color', "black");
            $('#lumiere').html('2');
        }
        else if ($('#lumiere:contains("2")').length) {
            $('body').css('background-color', "white");
            $('#lumiere').html('1');
        }
    });
});

answered Jun 28, 2013 at 15:19

nderscore

4,28125 silver badges29 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

Niet the Dark Absol Over a year ago

While I commend you for actually explaining the problem (unlike most jQuery-posting users), I have to say I am amused that while jQuery prides itself on achieving more with less code, I have managed to perform the same task as you without jQuery in LESS code.

nderscore Over a year ago

@Kolink I was merely answering the question asked. This is not the place to start a Vanilla vs jQuery debate.

Niet the Dark Absol Over a year ago

Yes it is RAEG... nah, I'm kidding. You get a +1, which I don't usually give to jQuery answers ;)

nderscore Over a year ago

@Baralai When the query returns no elements, it will have a length of 0. 0 evaluates to false in an if statement. Any non-zero number will evaluate as true.

Baralai Over a year ago

Got it. Thanks again! I'll be sure to come back and upvote as soon as I've unlocked that option.

|

Aurelio De Rosa · Accepted Answer · 2013-06-28 15:22:32Z

1

Refactoring a bit your code:

$(function() {
    $('#lumiere').click(function(event) {
        var $this = $(this);
        if ($this.text() === '1') {
            $('body').css('background-color', "black");
            $this.text('2');
        }
        else if ($this.text() === '2') {
            $('body').css('background-color', "white");
            $this.text('1');
        }
    });
});

The optimizations are:

bind() is old, use on() instead
Use the click() shortcut
No need to repeat selection inside your callback, just use this
The Id selector is the fastest one, don't miss this opportunity using the :contains filter
You don't need the filter at all because it isn't doing what you think. It test if the parameter you pass is contained in the text, not equal. So, if you need the latter, use text() instead.

answered Jun 28, 2013 at 15:22

Aurelio De Rosa

22.2k8 gold badges51 silver badges71 bronze badges

6 Comments

Niet the Dark Absol Over a year ago

If you're going to optimise using $this, surely you could also optimise the repeated call to .text()? :p

Aurelio De Rosa Over a year ago

Actually one time is used as getter and the second as setter. So how you'll optimize it?

Marian Bida Over a year ago

Are you serios, it is just 5 lines of code, whats wrong with you people.

Aurelio De Rosa Over a year ago

Sometimes also 1 line of code is worth for a discussion on best practices.

Baralai Over a year ago

I modified my code to take your input into account - used "click" with the .length suggested in the first answer. Unfortunately I can't upvote yet but I'll be sure to come back and do so as soon as I can!

|

War10ck · Accepted Answer · 2013-06-28 15:30:50Z

Glad to see you're using jQuery. It's a very powerful tool and a lot of fun to use. Try the following:

HTML:

<body>
    <button id="lumiere">1</button>
</body>

Keep your html as is.

CSS:

Add this class in the <head></head> section inside of a <style type="text/css"></style> block or in an external CSS file linked like so <link type="text/css" rel="stylesheet" href="external-sheet-name.css" />.

.bodyRed {
    background-color: red;
}

This class will be used below in the JavaScript logic.

JavaScript:

Modify your JavaScript like the following:

var $body = $('body'),
    $button = $('#lumiere');

$button.on('click', function(event) {
    if($body.hasClass('bodyRed')) {
        $body.removeClass('bodyRed');
        $button.text('2');
    } else {
        $body.addClass('bodyRed');
        $button.text('1');
    }
});

Explanation:

The first two statements store references to your two jQuery objects. This is useful because it provides a way for jQuery to reference the objects without re-traversing the DOM each time. This is more efficient in the long run.

The second modification to your code is to change bind to on. .on() is the newer, more efficient, and preferred method for attaching event handlers. The link above will take you to the documentation for .on().

The third modification is to remove the need for inline CSS by using the class created above. This also simplifies the logic. If the <body> tag has the class, remove it and change the button text. Otherwise, add it and then modify the button text.

That's all there is too it. To see it running in action, check out this live fiddle.

Good luck and happy coding! :)

Marcellus Frota · Accepted Answer · 2013-06-28 18:22:15Z

1

There's no need so much complications...

$(function() {
    $('#lumiere').bind('click', function(event) {
            var n = $('#lumiere').text();
            switch (n) {
            case '1':
                $('body').css('background-color', "black");
            $('#lumiere').text(2);
                break;
            case '2':
                $('body').css('background-color', "white");
            $('#lumiere').text(1);
                break;
            }            
    });
});

answered Jun 28, 2013 at 18:22

Marcellus Frota

111 bronze badge

1 Comment

Marcellus Frota Over a year ago

sorry for bad indentation!

Marian Bida · Accepted Answer · 2013-06-28 15:19:48Z

0

$(function() {
$('#lumiere').bind('click', function(event) {
     var v = parseInt($('#lumiere').text());

    if (v === 1) {
        $('body').css('background-color', "black")
        $('#lumiere').html('2')
    }
    else if (v === 2) {
        $('body').css('background-color', "white")
        $('#lumiere').html('1')
    }
}
);

} );

answered Jun 28, 2013 at 15:19

Marian Bida

1336 bronze badges

Comments

Niet the Dark Absol · Accepted Answer · 2013-06-28 15:20:09Z

0

I would like to suggest you use Vanilla JS for this task, it is much easier to use and much more efficient:

window.onload = function() { // if you place the script after the element,
                             // you can remove this line and the corresponding }
    document.getElementById('lumiere').onclick = function() {
        document.body.style.backgroundColor = this.firstChild.nodeValue == 1 ? "black" : "white";
        this.firstChild.nodeValue = 3-this.firstChild.nodeValue;
    };
};

answered Jun 28, 2013 at 15:20

Niet the Dark Absol

326k86 gold badges480 silver badges604 bronze badges

5 Comments

Niet the Dark Absol Over a year ago

This has the added benefit of respecting DRY ^_^

Baralai Over a year ago

Thanks a lot for your involvement Kolink. As soon as I'm done learning jQuery correctly, I'll be sure to take a look at Vanilla JS ;)

Niet the Dark Absol Over a year ago

You should really learn Vanilla before you even consider jQuery, especially since jQuery uses Vanilla.

Baralai Over a year ago

OK I'm really stupid, I thought "Vanilla JS" was another framework based on JS... but it IS JS! You're of course right, I do have some notions in JS but should really work more on it.

Niet the Dark Absol Over a year ago

Hehe, I love the joke, it's funny to see people's reactions when they realise that ;) Seriously, though, my opinion is: "If you don't know Vanilla, then you shouldn't use jQuery. If you do know Vanilla, then you don't need jQuery."

Collectives™ on Stack Overflow

If/else not working with click event in jQuery

6 Answers 6

7 Comments

6 Comments

Comments

1 Comment

Comments

5 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

7 Comments

6 Comments

Comments

1 Comment

Comments

5 Comments

Your Answer

Sign up or log in

Post as a guest

Related