0

I am trying to grab the href tag on the element clicked in the "navigation" div The current code returns "#BiblesandCommentaries" every time no matter what element I click in the navigation div.

html

<div class="navigation">
        <ul>
          <li><a href="#BiblesandCommentaries">الانجيل-تعليقات</a> </li>
          <li><a href="#EnquirersLibrary">الاستفسارات</a> </li>
          <li><a href="#NewBelievers">مؤمنون جدد</a> </li>
          <li><a href="#ChristianLiving">حياة المسيحى</a> </li>
          <li><a href="#FamilyLibrary">مكتبة الأسرة</a> </li>
          <li><a href="#YoungAdultLibrary">مكتبة الشباب</a> </li>
          <li><a href="#WorshipLibrary">مزامير وتراتيل</a> </li>
          <li><a href="#BibleTeachings">التدريس</a> </li>
          <li><a href="#Leadership">القيادة</a> </li>
          <li><a href="#SchoolofChrist">مدرسة الإنجيل</a> </li>
          <li><a href="#Dota">شاگرد سازی بصورت شنیدن</a></li>
          <li><a href="#MinorityLanguages">فهم المسيح</a> </li>
        </ul>
    </div>

js

$('.navigation').click(function() {
    test = $(this).find('a').attr('href');
    alert (test);
});
3
  • The reason you get the 'first' one is because you're are inside of "navigation" and a .find() stops at the first match case, so that's the first one every time. Mar 1, 2012 at 20:46
  • @Relic Actually .find will get all matches; it's .attr that stops at the first match. Mar 1, 2012 at 20:55
  • @mblase75 right, it's shifting off the first piece of the array it's returned. my bad... Mar 1, 2012 at 21:00

3 Answers 3

7

using .find().attr() like that will always get the first one, you need to change the selector for your click function to target the a like this:

$('.navigation a').click(function(e) {
    e.preventDefault(); //use this if you don't want to reload the page
    var test = $(this).attr('href');
    alert (test);
}); 

also you should declare your variables.

EDIT ok commenters, I was just fixing his code, but here ya go:

$('.navigation a').on("click",function(){
       alert($(this).attr('href'));
});

EDIT haha commenters, let's get pedantic:

$("div.navigation").on("click", "a", function(e) {
    e.preventDefault();
    alert ($(this).attr('href'));
});​​​​

without preventing the default operation of the a tag, you'll be reloading the page

EDIT: Want to take it a step further? Forget jQuery. Here's some vanilla JS

document.querySelector("div.navigation").onclick = function(e){
    if(e.target.href){
        e.preventDefault();
        alert(e.target.href);
    }
}
7
  • You shouldn't declare a variable here at all, you only use the jQuery object here once... Also, you shouldn't use '.click()' anymore where you can avoid it because it doesn't watch the DOM for new elements. only ones that exists when this code gets executed. Mar 1, 2012 at 20:51
  • This binds a handler to each anchor individually, which is unnecessary and slower (though the difference is likely imperceptible.)
    – Evan Davis
    Mar 1, 2012 at 20:51
  • @relic I think the alert is just to test. The actual goal is to store the value, as the original function references a global test var.
    – Evan Davis
    Mar 1, 2012 at 20:52
  • @ not imperceptible, read my answer about using the event vs adding handlers. Mar 1, 2012 at 21:02
  • @relic I've added an example just for you, because you're so special.
    – JKirchartz
    Mar 1, 2012 at 21:04
3

overall you should try this for best practices:

$('div.navigation').on("click", "a", function() {
    alert($(this).attr('href'));
});

Remember, try not to set variables in memory unless you have to parse the DOM more than once for the same element. Also always try and give as much depth to the jQuery selector as you can, gives it less of the DOM to parse. If that doesn't make sense, think of it this way. If it has to parse through only the div tags for the class 'navigation' that should remove all the other tags and take much less time than parsing every tag for it's class attribute.

or you could follow the event:

$('div.navigation').on("click", function(event) {
    if(event.target.href){ alert(event.target.href); }
});

The reason to do the above is for memory purposes as well. If you're only watching the wrapper, there is only one event listener in memory. If you have the event listener on the 'a' tag itself, you have to have an event listener in memory for every 'a' tag in the wrapper. This could get heavy... imagine if Google had an event listener on every image that loads in the images page... You'd run out of memory.

7
  • @Mathletics The only reason to set a location in memory is that you would need it later, right? Why take the 'time' MHz to do it if you're already setting the jQuery Object once? This way as soon as you have the result from the value of the selector and it's attribute, it's turned into a string, and then in this case set to the variable value inside the function call (alert) Mar 1, 2012 at 20:57
  • time is cheap, don't think it'll make any difference attaching events to a dozen links or declaring one var, if he was doing this on like 10k links it might be a different story. +1 for the code golf
    – JKirchartz
    Mar 1, 2012 at 21:09
  • @relic I'm assuming he'll need it later. Why would anyone want to just alert an href on click? Explicitly not storing values sounds like the worst kind of premature optimization.
    – Evan Davis
    Mar 1, 2012 at 21:13
  • I'm implying as good of standards as I can to the code snippet provided, and if you notice I agree with you when there is scope for the variable, if not don't set it. Memory, and CPU cycles, are what they are. And if you can always keep to the best standards you'll have less refactoring to do later. Mar 1, 2012 at 21:20
  • event.target.href for a hash-anchor will return the full URL string, domain + hash, I think he just wants the hash. (i've only tested in chrome, so it's not scripture)
    – JKirchartz
    Mar 1, 2012 at 21:20
0

.navigation is only one (ul). Try something like

$('.navigation a').click(function() {
    var test = $(this).attr('href');
    alert (test);
});

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.