current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

I posted this question in stackoverflow and they told me it would be better served here so here it is:

Is there a more effecient way of changing the mouseout state when a button is clicked? When a button is clicked I want the image in the click event to take precedence over the image in the mouseout event. The following code works but I'm not sure if nesting events is a good practice.

$("#targetBtn").on({
  "mouseover" : function() {
    $(this).find('img').attr('src', 'images/_hover.png');
  },
 "mouseout" : function() {
    $(this).find('img').attr('src', 'images/_mouseout.png');
 },
 "click" : function() {
   $(this).find('img').attr('src', 'images/_selected.png');
   $(this).mouseout(function() {
      $(this).find('img').attr('src', 'images/_selecetd.png');
   });
  }
});

If anyone could refactor this code and show me a better way of doing this it would be greatly appreciated. Also if someone could explain why nesting events is bad (if indeed it is) I would appreciate that as well.

share|improve this question
1  
Wait, why are you using jQuery and then just a few lines later using querySelector? – Downgoat Mar 16 at 15:11
    
sorry. I'm new to` JQuery` and I was just told in stackoverflow that its a bad practice. Why is it a bad practice if JQuery is just a= Javascript` library? – webDev Mar 16 at 15:14
up vote 4 down vote

It is bad practice as you add a new handler each time your event is fired.

To better understand this, try this:

$('p')
  .on('click', function(e) {
    console.log('1');
    $(this).on('click', function(e) {
      console.log('2');
    });
  });
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<p>
  click me
</p>

Each time you click "click me", it should log one more "2".

A better way is to refactor your code to use a flag

(function() {
    var flag = true;

    $('#targetBtn')
        .on('mouseover', function(e) {
            $(this).find('img').attr('src', 'images/_hover.png');
        })
        .on('mouseout', function(e) {
            if(flag)
                $(this).find('img').attr('src', 'images/_mouseout.png');
            else
                $(this).find('img').attr('src', 'images/_selected.png');

            // or
            // $(this).find('img').attr('src', (flag ? 'images/_mouseout.png' : 'images/_selected.png'));
        })
        .on('click', function(e) {
            flag = false;
            $(this).find('img').attr('src', 'images/_selected.png');
        });
})();

See this demonstration to grasp the result: 

(function() {
  var flag = true;

  $('#targetBtn')
    .on('mouseover', function(e) {
      console.log('src', 'images/_hover.png');
    })
    .on('mouseout', function(e) {
      if (flag)
        console.log('src', 'images/_mouseout.png');
      else
        console.log('src', 'images/_selected.png');

      // or
      //console.log('src', (flag ? 'images/_mouseout.png' : 'images/_selected.png'));
    })
    .on('click', function(e) {
      flag = false;
      console.log('src', 'images/_selected.png');
    });
})();
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<a href="#" id="targetBtn">btn</a>

share|improve this answer
    
Awesome explained. I hope you write more here. – st88 yesterday

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.