1
\$\begingroup\$

I've written my first completely self-written jQuery today. I just used the jQuery docs and I'm reasonably proud I got it to work, and working fine. It's not a very complex problem I'm solving, just an animated UI element, but it are my first steps, and I want to make sure I'm starting off right.

I think it's a lot of code for what I'm trying to achieve, and I'm looking for pointers or tips on how I could improve my code. I'm not at all averse to scrapping all jQuery here and doing it with pure JS either.

jQuery(document).ready(function () {
    event.preventDefault();
    var nav = jQuery(".navigation-menu");
    var button = jQuery(".navigation-title");

    function showNav() {
        nav.show('fast');
        button.removeClass('closed');
        button.addClass('open');
    }

    function hideNav() {
        nav.hide('fast');

        if (button.hasClass('open')) {
            button.removeClass('open');
        }

        button.addClass('closed');
    }

    hideNav();

    button.click(function () {
        if (button.hasClass('closed')) {
            showNav();
        } else if (button.hasClass('open')) {
            hideNav();
        } else if (!button.hasClass('open') || !button.hasClass('closed')) {
            hideNav();
        }
    });
});

Here's the fiddle: http://jsfiddle.net/yeawg/2/

I'm also wondering: am I using event.preventDefault() right?

\$\endgroup\$
0
3
\$\begingroup\$

I found .toggle() in the jQuery docs, and it seemed to me I could use that far better. I changed my code into:

jQuery(document).ready(function(){
    var nav = jQuery(".navigation-menu");
    var button = jQuery(".navigation-title");

    nav.toggle();

    button.click(function(event){
        event.preventDefault();
        nav.toggle('fast');
    });
});

This looks a lot leaner and better readable.

\$\endgroup\$
1
\$\begingroup\$

For one, it can be done CSS-only.

JS:

None at all. Remove your JS.

HTML:

<nav class="mobile-navigation">
    <!-- dummy target since CSS has no "prev" -->
    <i id="menu"></i>

    <div class="navigation-menu">
        <a id="menu-open" href="#menu">Menu</a>
        <a id="menu-close" href="#">Menu</a>
    </div>

    <ul id="menu-real" class="navigation-menu">
        <li><a href="#">Cafe</a></li>
        <li><a href="#">Toys</a></li>
        <li><a href="#">Shop-in-Shop</a></li>
        <li><a href="#">Feestjes</a></li>
        <li><a href="#">Agenda</a></li>
        <li><a href="#">Fietsverhuur</a></li>
    </ul>
</nav>

Add this CSS to your existing CSS:

/* menu animators */
#menu ~ #menu-real {
    height:0;
    width:0;
    opacity:0;
    box-sizing:border-box;
    position:absolute;
    overflow:hidden;
    transition: all 0.5s ease;
}
#menu:target ~ #menu-real {
    width: 100%;
    height : 100%;
    opacity : 1;
    position:relative;
}

/* Menu button swappers */
#menu ~ * > #menu-open {display:block}
#menu:target ~ * > #menu-open {display:none}

#menu ~ * > #menu-close {display:none}
#menu:target ~ * > #menu-close {display:block}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your answer, however I'm trying to learn more about JavaScript. I'm definitely looking in to your solution however. Seems the better option performance-wise. (Even though it probably won't matter much in this case, at least I don't have to load jQuery). \$\endgroup\$ Oct 26 '13 at 14:28
1
\$\begingroup\$

Some points:

  • The preventDefault doesn't do anything in a ready event handler, as there is no default action that would happen after the event. (If you would use the preventDefault method you should declare a parameter in the event handler to catch the event object sent to it.)
  • You don't need to check if an element has a class before removing it, if it's not there the removeClass method will just change nothing.
  • You can chain calls that work with the same jQuery object.
  • When checking what to do with the navigation, you can just use an else instead of checking every combination.

So:

jQuery(document).ready(function () {
  var nav = jQuery(".navigation-menu");
  var button = jQuery(".navigation-title");

  function showNav() {
    nav.show('fast');
    button.removeClass('closed').addClass('open');
  }

  function hideNav() {
    nav.hide('fast');
    button.removeClass('open').addClass('closed');
  }

  hideNav();

  button.click(function () {
    if (button.hasClass('closed')) {
        showNav();
    } else {
        hideNav();
    }
  });

});
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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