Fading and Loader jQuery Improvement

Question

I'm not much of a jQuery coder (more of a PHP coder), except to call a few get values out of class and id and Ajax calls. However, I need to make a checkout system work and one thing I would like to do is to put a loading wait while I send a form through Ajax. When it's finished, it would redirect the user.

So on click:

1. load the loading spinner
2. fade the background out
3. Ajax call
4. when Ajax is done submitting a form which indirectly redirects.

Here is my code, following why I don't like it:

jQuery.fn.center = function () {
 this.css("position","absolute");
 this.css("top", ( $(window).height() - this.height() ) / 2+$(window).scrollTop() + "px");
 this.css("left", ( $(window).width() - this.width() ) / 2+$(window).scrollLeft() + "px");
 return this;
 }
 $( ".buttonFinish" ).on('click', function(event) {
 $('body').fadeOut('slow');
 $('#loader').center();
 $('#loading').center();
 $('#loader').show();
 $('#loading').show();
 //Use the above function as:
 var request = $.ajax({
 url:'{{ URL::route('saveCart') }}',
 type:'POST'
 });
 request.done(function(msg) {
 $('.order_number').val(msg);
 $('#summaryForm').submit();
 event.preventDefault();
 });
 return false;
 });

Let's get a few questions out of the way: {{UR::route('')}} is laravel syntax to call the routing name for URL. loader/loading are two div that I have for the spinner one is text the other is an animation through CSS. (I'd rather CSS it than to load a pix, but I am aware that loader.gif can be small)

My issue (and feeling) that this can be better. I am using CSS with display:none to hide my two div, and then I call fadeOut, show, show, and center center function to center my loading Ajax in order to center things. Is there a compact way to achieve all the 4 steps needed in a clean way?

• Flambino: Does this code actually work? As far as I can tell, you hide (fade out) the body element, which hides everything. It doesn't matter that you call .show() on something later: it'll remain hidden because body is hidden.
• azngunit81: @flambino yes the code does work. As mentioned the first thing it does is hide the body, then pops the two div #loader #loading which in on the foreground with .show()
• Flambino: I see you've gotten a pretty good answer, but just to reiterate: There is no "foreground" that's not nested inside the body element. Again, everything you see on the page is inside the body element, so if you hide the body, you hide everything.
• azngunit81: @Flambino after reading it more throughly - what i saw as result was just a fast pace show() but clearly got masked by the fadedout...despite it seems that its appears in foreground as you have suggested it was not

Answer 1

Let's go through it one section at a time.

jQuery.fn.center = function () {
 this.css("position","absolute");
 this.css("top", ( $(window).height() - this.height() ) / 2+$(window).scrollTop() + "px");
 this.css("left", ( $(window).width() - this.width() ) / 2+$(window).scrollLeft() + "px");
 return this;
 }

• You don't have to explicitly append 'px' to your numeric values. jQuery is smart enough to correctly format your CSS styles.

• Since this is a jQuery plugin, you return this; at the end of the function. This is good, as it preserves chaining.

• If the .center function is only used for the loading divs, consider removing it and instead styling the divs with plain CSS.

  #loader {
   position: fixed;
   top: 50%;
   left: 50%;
   margin-top: -50px; /* half of the height */
   margin-left: -100px; /* half of the width */
  }
  ...
  

---

$( ".buttonFinish" ).on('click', function(event) {
 $('body').fadeOut('slow');
 $('#loader').center();
 $('#loading').center();
 $('#loader').show();
 $('#loading').show();

• As mentioned by @Flambino, this particular code (as presented in the question) will fade out the entire body of the HTML document, which includes everything, even the loader elements. Perhaps the code that you're actually using is slightly different (is the selector .body or #body instead?).

• It's best practice to enclose JavaScript that uses external dependencies inside an IIFE. By doing so, you can use the "safe" name of an external dependency while still being able to alias is to a more convenient identifier locally. In addition, you can freely create variables inside the scope of the IIFE without polluting the global namespace. Your example code is fairly short, but you likely have much more code on the page, and it is a good habit to have regardless. Here's one way to do it:

  (function($) {
   $( ".buttonFinish" ).on('click', function(event) {
   ...
   });
  })(jQuery);
  

• You can select both elements at the same time, just like in CSS: $('#loader, #loading').

• You can chain the .show() and .center() together: $('...').show().center().

---

 //Use the above function as:
 var request = $.ajax({
 url:'{{ URL::route('saveCart') }}',
 type:'POST'
 });
 request.done(function(msg) {
 $('.order_number').val(msg);
 $('#summaryForm').submit();
 event.preventDefault();
 });
 return false;
});

• Since you're not using the request variable for anything other than .done(), you can skip declaring it and simply chain .done() after $.ajax():

  $.ajax({
   ...
  }).done(function(msg) {
   ...
  });
  

As an overall note, the indentation of the code (as it is in the question) seems haphazard, making the control flow more difficult to see than it could be. Making matching braces have the same indentation (and, in general, maintaining a consistent style) will help make the code more readable and understandable.

• azngunit81: appreciate the thorough input. As far as the indentation thats primarily due to only hitting ctrl+k after a copy paste. The center is only to center the div. If I style it does your code snippet ensures the loader is within view of the user (ie the user has scrolled down and the loader activates with the jquery its perfectly centered). As far as the body.fadeOut vs show() the intention was to make the body of the page be greyed out (pushed to the background even further) and have the loading more on the forefront (as mentioned in my goal). If its an incorrect method - please correct me. tks.
• voithos: @azngunit81: Yes, position: fixed; does exactly what you're describing: it 'fixes' an element absolutely within the window (regardless of scroll position), instead of absolutely within the page. As for fadeOut, indeed, this method fade out and eventually completely hides its target. Instead, you could group the main elements within your page inside something like <div id="page-main">, add #header outside of this div, and then use something like jQuery.fn.fadeTo('slow', 0.6) on #page-main to fade to 60% opacity.
• azngunit81: I see, one last question: is there any degradation in performance if two div requires a show()? (like th #loader, #loading)
• voithos: @azngunit81: No, .show() is a very fast operation (it's essentially just setting the display CSS style). However, if one of the elements is, display-wise, "contained" within the other one, you could actually just nest it within the outer one, and then you would only have to .center() and .show() the outer container.
• azngunit81: I tried that but the css i am using fpr #loader is animation: orbit 7.15s infinite; (basically little balls that rotates around a radius) and I wanted the text "loading" to be centered. The only way I found to do that effectively while having both correctly centered was to div both and center both.