I am making a movie review web site as a project for school, and I want to put a click function on the image movie cover which will load the details and reviews of that movie. The code I'm using works but does not seem practical. The parameter in my loadReviews function is the movie ID for the database.
$(document).ready(function () {
$("#cover1").click(function () { loadReviews(1); });
$("#cover2").click(function () { loadReviews(2); });
$("#cover3").click(function () { loadReviews(3); });
$("#cover4").click(function () { loadReviews(4); });
$("#cover5").click(function () { loadReviews(5); });
$("#cover6").click(function () { loadReviews(6); });
$("#cover7").click(function () { loadReviews(7); });
$("#cover8").click(function () { loadReviews(8); });
$("#cover9").click(function () { loadReviews(9); });
$("#cover10").click(function () { loadReviews(10); });
$("#cover11").click(function () { loadReviews(11); });
$("#cover12").click(function () { loadReviews(12); });
});
As you can see I am writing each one manually. I tried using a for loop like this but does not work the way I thought.
for (i = 1; i < 12; i++) {
$("#cover" + i).click(function () { loadReviews(i); });
}
Using the loop it makes each image load the details of the same (#12) movie. Each image is assigned the class 'cover1', 'cover2' etc. I want some sort of 'loop' to automatically bind each click event to the correct image cover. I'm using a generic handler in Visual Studio 15. We must use ajax and jquery to update the page without a postback, this is how I am getting the movies and reviews.
If I need to show more code let me know. Thanks!
You could get away with having just one click handler and store the identifier as a data attribute. So your repeated HTML element might be something like this:
<div class="cover" data-movieid="1">
...
</div>
<div class="cover" data-movieid="2">
...
</div>
etc.
Then assign a single click handler and within that handler get the identifier from the element which was clicked. Something like this:
$(document).ready(function () {
$('.cover').click(function () {
var movieID = $(this).data('movieid');
loadReviews(movieID);
});
});
Depending on what loadReviews() does, you can probably make the whole thing simpler. Instead of giving everything ids and only using those in your selectors, from any given clicked element you can use jQuery to query the DOM and find the relative element you need without using an id.
If the HTML can't be changed
$("[id^=cover]").click(function () {
var rev = parseInt(this.id.replace(/\D+/g, ''));
loadReviews(rev);
});
Instead of using IDs for each cover I would recommend using a "cover" class and a data parameter with the id.
<div class"cover" data-movieid="1"></div>
and the js code would look like:
$(document).ready(function () {
$(".cover").click(function () { loadReviews($(this).data('movieid')); });
});
Using the loop it makes each image load the details of the same (#12) movie.
This happens because when the loop ends the value of the variable is always the last while the event will happen in future.
Two ways to solve it (Immediately-invoked function expression):
function loadReviews(i) {
console.log(i);
}
for (i = 1; i < 5; i++) {
(function(i) {
$("#cover" + i).on('click', function (e) {
loadReviews(i);
});
}(i));
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<button id="cover1">Cover 1</button>
<button id="cover2">Cover 2</button>
<button id="cover3">Cover 3</button>
<button id="cover4">Cover 4</button>
<button id="cover5">Cover 5</button>
The second way is based on the id (i.e: get the last part id: remove the cover string):
function loadReviews(i) {
console.log(i);
}
for (i = 1; i < 5; i++) {
$("#cover" + i).click(function () {
loadReviews(this.id.replace('cover', ''));
});
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<button id="cover1">Cover 1</button>
<button id="cover2">Cover 2</button>
<button id="cover3">Cover 3</button>
<button id="cover4">Cover 4</button>
<button id="cover5">Cover 5</button>
You are facing the typical "callback inside look" context problem.
Let's check the code.
Having this:
$("#cover1").click(function () {
loadReviews(1);
});
Means that the code inside the function will be run only when the click event occurs, and not when the event is attached, so the runtime will go this way:
// First, attaches the event
$("#cover1").click(function () {
// This will be run when user clicks, so will be called last
loadReviews(1);
});
// The runtime will continue right BEFORE the click callback is run
So in your loop the runtime will go this way:
for (i = 1; i < 12; i++) {
// var i value starts from 0. And attaches the click event
$("#cover" + i).click(function () {
// Runs after all the loop is done (when the i value is 12)
loadReviews(i);
});
// loop continue BEFORE the content of the click callback is run
}
One fast way to fix your loop is to call a function with the variable declared inside the function, so it will be never changed by external actions.
function attachClick(id) {
// This will be run on each loop, creating a id variable
// that will be unique for this context
$("#cover" + id).click(function () { loadReviews(id); });
}
for (i = 1; i < 12; i++) {
// Call the function passing the i variable as parameter
attachClick(i);
}
And the way that you will se a lot around there is by creating anonymous functions (does exactly the same as above but without creating a named function elsewhere):
for (i = 1; i < 12; i++) {
(function(id) {
$("#cover" + id).click(function () {
loadReviews(id);
});
})(i);
}
Those are the fastest ways of modifying your code to have a working one, but when working with events that way, the best is to attach only one event to the parent, capture the element clicked and get the id from there. The less events attached, the faster is everything.
