0

I can't see what I'm doing wrong here.. Can anyone help please?

JavaScript:

function toggleMainNav() {
        var navLink = document.getElementsByClassName('nav_link')[0];
        var mainNav = navLink.nextSibling;
        if ( mainNav.className.match(/(?:^|\s)inactive(?!\S)/) ){
            mainNav.className = 'active';
        } else{
            mainNav.className = 'inactive';
        }
    }

    document.getElementsByClassName('nav_link')[0].addEventListener( 'click' , toggleMainNav );

this is the HTML:

<a class="nav_link">☰ Menu</a>
<ul class="inactive">
</ul>
Stephen Docy
  • 4,738
  • 7
  • 18
  • 31
Pepijn Gieles
  • 619
  • 14
  • 23
  • 1
    can you provide jsfiddle? – justtal May 27 '14 at 12:30
  • hi dear friends add your code in jsfiddle [link] (http://jsfiddle.net/) – Prashant May 27 '14 at 12:30
  • 1
    **If using modern browsers only**, you may also consider `element.classList.contains("someClassName")` instead of a regEx. `element.classList` has a number of nice (read: useful) member functions. Much nicer to work with than `element.className` directly. – enhzflep May 27 '14 at 12:52

1 Answers1

3

nextSibling will be a text node containing whitespace. Either scan until you get nodeType === 1 or use nextElementSibling (but check whether it's supported on your target browsers).


Side note: getElementsByClassName has worse support than querySelector / querySelectorAall (IE8 has the latter but not the former, for instance), so you might consider using those instead.

Side note 2: IE8 also doesn't have addEventListener.

Side note 3: If you hook up your handler via addEventListener, within the handler this will already be the first nav_link, so you don't have to look it up again.

Side note 4: Some older browsers will fail if you don't give the third argument to addEventListener (it didn't used to be optional). To be broadly-compatible, be sure to include the false at the end.

T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
  • Thanks! Clear answer. – Pepijn Gieles May 27 '14 at 12:32
  • Is it better to avoid using addEventListener and add a onclick attribute? – Pepijn Gieles May 27 '14 at 12:44
  • 1
    @PepijnGieles: No, because using `onclick` means you can't have multiple handlers on the same element. But if you need to support IE8 or earlier, you have to work around the fact that it doesn't have it. [This answer](http://stackoverflow.com/a/23799448/157247) shows one way to do that. – T.J. Crowder May 27 '14 at 12:52
  • By the way, I need this action only to happen on mobile devices. Since it's to show/hide the menu. Do you know where I can find the addEventListener support for those browsers? – Pepijn Gieles May 27 '14 at 12:58
  • @PepijnGieles: http://caniuse.com has a very good database, but doesn't have a page on `addEventListener`. [MDN](https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener) suggests reasonable support. Side note: I've added a fourth "side note" to the answer. :-) – T.J. Crowder May 27 '14 at 13:07
  • @TJCrowder Thanks for the tips, I knew about canIuse but indeed couldn't find it. – Pepijn Gieles May 27 '14 at 13:22