0

I have following jQuery code

$(document).ready(function() { 
    for(var i=1;i<4;i++) { 
        var img= ".img"+i; 
        var p= ".p"+i;
        $(img).on("click", function(){
            $(p).hide(); 
        }); 
    } 
});

Applied on the following html:

<div>
    <article>
         <h1>Over mezelf <img src='images/plus.png' class='img1'/></h1>

        <p class='p1'>Test</p>
    </article>
    <article>
         <h1>Contact <img src='images/plus.png' class='img2'/></h1>

        <p class='p2'>Test</p>
    </article>
    <article>
         <h1>Website <img src='images/plus.png' class='img3'/></h1>

        <p class='p3'>Test</p>
    </article>
</div>

When I click on the images, the last <p> disappears, it doesn't work on the other ones.

iCollect.it Ltd
  • 92,391
  • 25
  • 181
  • 202
stijn.aerts
  • 6,026
  • 5
  • 30
  • 46
  • Have a look at http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example for an explanation of the problem. – Felix Kling Jun 02 '14 at 15:21

4 Answers4

4

The problem is due to the scope of p. When it comes round to running the event handler code it will always use the most recent value of 'p', which in this case would be ".p3" (see here for more information)

Quick Fix

You could use a closure instead:

$(document).ready(function() { 
    for(var i = 1; i < 4; i++) { 
        var img= ".img" + i;
        var p = ".p" + i;
        (function(p){
            $(img).on("click", function(){
                $(p).hide();
            });
        })(p);
    }
});

Here is a working example

Recommended

Personally I would prefer to change your HTML to use a common class, this will avoid the need for a loop. You can then navigate your way to the correct p tag...

HTML:

<div>
    <article>
         <h1>Over mezelf <img src='images/plus.png' class='img'/></h1>

        <p class='p1'>Test</p>
    </article>
    <article>
         <h1>Contact <img src='images/plus.png' class='img'/></h1>

        <p class='p2'>Test</p>
    </article>
    <article>
         <h1>Website <img src='images/plus.png' class='img'/></h1>

        <p class='p3'>Test</p>
    </article>
</div>

(note that you can have multiple classes if you still need the numbered ones for something else. e.g: "img img1")

Javascript:

$(document).ready(function() {
    $(".img").on("click", function () {
        $(this).closest("article").find("p").hide();
    });
});

Here is a working example

Community
  • 1
  • 1
musefan
  • 47,875
  • 21
  • 135
  • 185
  • `For` loops are seldom needed with jQuery events. Always best to try and roll up into a single selector if possible. I would recommend they not use unique class names as that is kind of pointless. Unique IDs and/or shared classes would make more sense (if needed at all). Adding an IIFE is complicating further something that should be trivial. – iCollect.it Ltd Jun 02 '14 at 15:34
  • @TrueBlueAussie: Yes I would have gone down that path, and was tempted to included it in my answer but I was undecided if it was going a bit too far considering the question – musefan Jun 02 '14 at 15:40
  • The original code needs to be printed out, then shredded and recycled! This was putting on a bandaid when amputation was needed :) – iCollect.it Ltd Jun 02 '14 at 15:43
4

You just need this

$(img).on("click", function(){
    $(this).closest('article').find('p').hide(); 
 });

Fiddle

Docs

  • closest()
  • find()
    • Dhaval Marthak
    • 17,246
    • 6
    • 46
    • 68
    • `For` loops are seldom needed with jQuery events. Always best to try and roll up into a single selector if possible. I would recommend they not use unique class names as that is kind of pointless. Unique IDs and/or shared classes would make more sense (if needed at all). – iCollect.it Ltd Jun 02 '14 at 15:34
    1

    You can simply use:

     $('img').on("click", function(){
     $(this).parent().next().hide(); 
 });

    JSFiddle: http://jsfiddle.net/TrueBlueAussie/XhRPC/1/

    iCollect.it Ltd
    • 92,391
    • 25
    • 181
    • 202
    Milind Anantwar
    • 81,290
    • 25
    • 94
    • 125
    1

    Milind Anantwar's answer is definitely the way to go, but here are a few more notes:

    You may need the selectors to be more specific, so they are less dependent on layout changes:

    JSFiddle: http://jsfiddle.net/TrueBlueAussie/XhRPC/2/

    $(function () {
     $('article h1 img').on('click', function(){
         $(this).closest('article').find('p').last().hide(); 
     });
});

    This finds the closest article ancestor, then finds the last paragraph (.children('p').last() does the same thing only faster than find as it search only one level)

    *Note: $(function(){YOUR CODE HERE}); is just a shortcut for $(document).ready(function(){YOUR CODE HERE});

    You should not have unique class names unless you actually mean to style each element specifically. Unique identifiers belong in id= and shared classes belong in class=.

    Dynamic elements?: http://jsfiddle.net/TrueBlueAussie/XhRPC/3/

    Should you introduce dynamically loaded elements, of the same type you have shown, you can easily change it to use a delegated version of on():

    $(function () {
     $(document).on('click', 'article h1 img', function(){
         $(this).closest('article').find('p').last().hide(); 
     });
});

    Delegated events listen at an ancestor, then apply the selector, then apply the function to any matching elements that generated the event.

    The preference is to use the first non-changing ancestor in your DOM. Failing that (you do not show the rest of the page) you can use document which receives all bubbled events on the page.

    iCollect.it Ltd
    • 92,391
    • 25
    • 181
    • 202