Reputation: 6206
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.
Upvotes: 0
Views: 98
Reputation: 93631
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:
$(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=
.
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.
Upvotes: 1
Reputation: 48465
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);
}
});
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();
});
});
Upvotes: 4
Reputation: 82251
You can simply use:
$('img').on("click", function(){
$(this).parent().next().hide();
});
Upvotes: 1
Reputation: 17366
You just need this
$(img).on("click", function(){
$(this).closest('article').find('p').hide();
});
Docs
closest()
find()
Upvotes: 4