1
\$\begingroup\$

The function is to click a button, the browser window then scrolls to the top of the page, then a class is added to a menu item (with CSS to make the sub menu appear). Then when the cursor leaves the sub menu, the class is removed to allow the sub menu to disappear.

Can I make the following code shorter or more efficient? I have written it to work with Joomla and jQuery with no conflict.

jQuery(document).ready(function(){
    jQuery("#instruct-us-readmore").click(function(){
        jQuery('html, body').animate({scrollTop:0}, 800);
        setTimeout(
        function() {
            jQuery("#iceMenu_144 ul.sub_level_1").addClass("instruct-button-click");
        },950);


     });
});


  jQuery(document).ready(function(){
        jQuery("#iceMenu_144 ul.sub_level_1").mouseleave(function(){
            jQuery("#iceMenu_144 ul.sub_level_1").removeClass("instruct-button-click");
    });
      });
\$\endgroup\$
  • \$\begingroup\$ shorter is not necessarily more efficient... \$\endgroup\$ – pppp Sep 29 '16 at 15:02
  • \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Sep 29 '16 at 15:10
  • \$\begingroup\$ Ok, so you wouldn't suggest any amendments? \$\endgroup\$ – ADRIAN Sep 29 '16 at 15:10
1
\$\begingroup\$

jQuery provides itself as first argument in the ready callback. You can assign it the $ name so you can use it shorthand inside the callback.

jQuery(function($){
  // Use jQuery as $ inside here
});

Also, you don't need multiple ready handlers. Whenever possible, just bunch them up into one.

jQuery(function($){
  // Click handler
  // Mouse handler
});

Next is that we don't know for certain how jquery.animate schedules animation. This might affect how sooner or later the callback for setTimeout is executed.

However, jquery.animate accepts a function as the last argument which is run when the animation is complete. Use that instead of doing a timeout.

$('html, body').animate({scrollTop:0}, 800, function(){
  // done animating
});
\$\endgroup\$

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.