patternjavascriptMinor
First time jQuery on xkcd
Viewed 0 times
xkcdfirstjquerytime
Problem
I'm a first time user of jQuery (and a huge fan already). I am trying to make the comic picture of an xkcd page
Here's what I've done.
Is this efficient? Is this robust? Any jQuerying tips?
onclick to the explanation page.Here's what I've done.
$("#comic").click(function(){
return window.open("http://www.explainxkcd.com/wiki/index.php/" +
document.URL.split("/")[3]);
});Is this efficient? Is this robust? Any jQuerying tips?
Solution
Besides not needing to return the result of
same as calling both
the passed jQuery.Event object.
normal (non-jQuery) event handlers, in which, notably,
Clearly, thats not what we want, so instead of returning the result of
This may not be necessary, but I would be more comfortable using
over
mainly because
Indentation would also help the code be more readable, however it's short so it doesn't matter too much... That said, I would write your code:
window.open, it looks pretty good. See this thread for an explanation of what returning a value from event does. From this answer,return false from within a jQuery event handler is effectively thesame as calling both
e.preventDefault and e.stopPropagation onthe passed jQuery.Event object.
e.preventDefault() will prevent the default event from occuring,e.stopPropagation() will prevent the event from bubbling up andreturn false will do both. Note that this behaviour differs fromnormal (non-jQuery) event handlers, in which, notably,
return false does not stop the event from bubbling up.Clearly, thats not what we want, so instead of returning the result of
window.open, we should just omit the return.This may not be necessary, but I would be more comfortable using
var comicID = location.pathname.split("/")[1];over
var comicID = document.URL.split("/")[3]);mainly because
http:// may be omitted from the URL and the code will still work (shouldn't ever be a problem either way). It's also more intuitive. I would also leave a comment for the code such as// where path name looks like /156/ for xkcd.com/156/Indentation would also help the code be more readable, however it's short so it doesn't matter too much... That said, I would write your code:
// when a user clicks the comic open the explanation
$("#comic").click(function(){
// where pathname is the comicID eg /156/ for xkcd.com/156
window.open("http://explainxkcd.com/wiki/index.php/" + location.pathname.split("/")[1]);
});Code Snippets
var comicID = location.pathname.split("/")[1];var comicID = document.URL.split("/")[3]);// where path name looks like /156/ for xkcd.com/156/// when a user clicks the comic open the explanation
$("#comic").click(function(){
// where pathname is the comicID eg /156/ for xkcd.com/156
window.open("http://explainxkcd.com/wiki/index.php/" + location.pathname.split("/")[1]);
});Context
StackExchange Code Review Q#51098, answer score: 8
Revisions (0)
No revisions yet.