6
\$\begingroup\$

I have a link (avatar)#qam-account-toggle and div.qam-account-items, the div containing user account links. I am toggling div on avatar click as well as click anywhere but div.qam-account-items

Here is my code. How can I optimize this better?

$('#qam-account-toggle').click(function(e){
 e.stopPropagation();
 $(this).toggleClass('account-active');
 $('.qam-account-items').fadeToggle('fast');
});
$(document).click(function(){
 $('#qam-account-toggle.account-active').removeClass('account-active');
 $('.qam-account-items:visible').hide();
});
$('.qam-account-items').click(function(event){
 event.stopPropagation();
});
palacsint
30.4k9 gold badges82 silver badges157 bronze badges
asked Mar 29, 2014 at 16:59
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

I'd suggest wrapping the code in a function, so it's all in one place and can share variables. Then there's no need to find and re-find the same elements in each event handler.

Also, you're doing some extra filtering by class name and such which is unnecessary. E.g. .hide() will hide whatever's visible, so you don't need to first use :visible to find the visible elements.

function enableAccountItemsToggling() {
 // find these once
 var accountToggle = $('#qam-account-toggle'),
 accountItems = $('.qam-account-items');
 // toggle items
 accountToggle.on("click", function (event) {
 event.stopPropagation();
 accountToggle.toggleClass('account-active');
 accountItems.fadeToggle('fast');
 // a single click is enough; no need for a persistent event handler
 // so we use .one() instead of .on()
 $(document).one("click", function () {
 accountToggle.removeClass("account-active");
 accountItems.fadeOut("fast");
 });
 });
 accountItems.on("click", function (event) {
 event.stopPropagation();
 });
}
answered Mar 29, 2014 at 18:59
\$\endgroup\$
1
\$\begingroup\$

Not much to say here, it looks pretty good. I've moved to only using .on for attaching events as I think it encourages better practices. Remember it's better to save selectors as variables if you are going to reuse them.

Instead of catching the click on .qam-account-items you can just fire the event on everything but those classes;

$(document).on('click', '*:not(.qam-account-items)', function() {
 $('#qam-account-toggle.account-active').removeClass('account-active');
 $('.qam-account-items:visible').hide();
});
answered Mar 29, 2014 at 19:50
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for the answer but this code is not working. stopped show hide.. any idea why is that? \$\endgroup\$ Commented Mar 31, 2014 at 16:08
  • \$\begingroup\$ I see this code is only for ignoring qam-account-items but it is still hiding it when I click on input box within the div \$\endgroup\$ Commented Mar 31, 2014 at 16:18
  • \$\begingroup\$ @pixelngrain Yeah you're right... I'll mull it over. \$\endgroup\$ Commented Mar 31, 2014 at 18:44
  • \$\begingroup\$ Any idea how to fix it? \$\endgroup\$ Commented Apr 3, 2014 at 7:19

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.